| Message ID | 20210315095710.7140-1-henning.schild@siemens.com |
|---|---|
| Headers | show |
| Series | add device drivers for Siemens Industrial PCs | expand |
Hi, On 3/15/21 11:14 AM, Henning Schild wrote: > Am Mon, 15 Mar 2021 10:57:10 +0100 > schrieb Henning Schild <henning.schild@siemens.com>: > >> 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 >> <michael.haener@siemens.com> Signed-off-by: Henning Schild >> <henning.schild@siemens.com> --- >> drivers/platform/x86/pmc_atom.c | 47 >> +++++++++++++++++++-------------- 1 file changed, 27 insertions(+), >> 20 deletions(-) >> >> diff --git a/drivers/platform/x86/pmc_atom.c >> b/drivers/platform/x86/pmc_atom.c index ca684ed760d1..38542d547f29 >> 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,23 @@ static void pmc_dbgfs_register(struct pmc_dev >> *pmc) } >> #endif /* CONFIG_DEBUG_FS */ >> >> +static bool pmc_clk_is_critical = true; >> + >> +static int siemens_clk_is_critical(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 1; >> + >> +out: >> + pmc_clk_is_critical = false; >> + return 1; >> +} >> + >> /* >> * Some systems need one or more of their pmc_plt_clks to be >> * marked as critical. >> @@ -424,24 +442,10 @@ static const struct dmi_system_id >> critclk_systems[] = { }, >> }, >> { >> - .ident = "SIMATIC IPC227E", >> - .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", >> + .callback = siemens_clk_is_critical, >> + .ident = "SIEMENS AG", >> .matches = { >> DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"), >> - DMI_MATCH(DMI_PRODUCT_VERSION, >> "A5E45074588"), }, >> }, >> >> @@ -453,7 +457,7 @@ 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); >> + const struct dmi_system_id *d; >> >> clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL); >> if (!clk_data) >> @@ -461,9 +465,12 @@ 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)) { > > Had to switch to check_system to get the callback to work. > >> + clk_data->critical = pmc_clk_is_critical; >> + if (clk_data->critical) { >> + d = dmi_first_match(critclk_systems); >> + pr_info("%s critclks quirk enabled\n", >> d->ident); > > Now need a double match here just to print the ident. Not too happy > with that but proposing it like this to keep the ident printing. > > I guess it could be improved by not printing the ident or having a > global variable and global callback to remember the ident to print > later. I would propose to not print the ident if the double-match does > not find traction. IMHO it would be best to add another callback for the non Siemens entries which just prints the ideent and returns 1 to avoid needsly looping over the rest of the array. And then just set the callback member of all the non Siemens entries to this new callback. The space for the callback pointer is already reserved in the struct anyways, so actually setting it does not take up any space. Regards, Hans
Am Mon, 15 Mar 2021 12:55:13 +0200 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > On Mon, Mar 15, 2021 at 12:12 PM Henning Schild > <henning.schild@siemens.com> wrote: > > > > 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. > > Thanks for an update! > > I did review more thoughtfully the series and realized that you can > avoid that P2SB thingy and may the approach be much cleaner if you > register the real GPIO driver and convert your LEDs to be a GPIO LEDs. > Then you won't need any custom code for it (except some board file, or > what would be better to file _DSD in your ACPI tables. Thanks Andy. I will need to read through your comments and existing code. Are you saying there already is a GPIO driver that i should rather hook into ... given there is and will not be WDAT any time soon? Can you please point it out to me, the driver and maybe an example. If you are suggesting to write a generic GPIO driver, i would probably say that this can hopefully wait until we have a second user and need that level of abstraction. regards, Henning > -- > With Best Regards, > Andy Shevchenko
Am Mon, 15 Mar 2021 12:31:11 +0200 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > On Mon, Mar 15, 2021 at 12:02 PM Henning Schild > <henning.schild@siemens.com> wrote: > > > > This mainly implements detection of these devices and will allow > > secondary drivers to work on such machines. > > > > The identification is DMI-based with a vendor specific way to tell > > them apart in a reliable way. > > > > Drivers for LEDs and Watchdogs will follow to make use of that > > platform detection. > > ... > > > +static int register_platform_devices(u32 station_id) > > +{ > > + u8 ledmode = SIMATIC_IPC_DEVICE_NONE; > > + u8 wdtmode = SIMATIC_IPC_DEVICE_NONE; > > + int i; > > + > > + platform_data.devmode = SIMATIC_IPC_DEVICE_NONE; > > + > > + for (i = 0; i < ARRAY_SIZE(device_modes); i++) { > > + if (device_modes[i].station_id == station_id) { > > + ledmode = device_modes[i].led_mode; > > + wdtmode = device_modes[i].wdt_mode; > > + break; > > + } > > + } > > + > > + if (ledmode != SIMATIC_IPC_DEVICE_NONE) { > > + platform_data.devmode = ledmode; > > + ipc_led_platform_device = > > + platform_device_register_data(NULL, > > + KBUILD_MODNAME "_leds", > > PLATFORM_DEVID_NONE, > > + &platform_data, > > + sizeof(struct > > simatic_ipc_platform)); > > + if (IS_ERR(ipc_led_platform_device)) > > + return PTR_ERR(ipc_led_platform_device); > > + > > + pr_debug("device=%s created\n", > > + ipc_led_platform_device->name); > > + } > > + > > + if (wdtmode != SIMATIC_IPC_DEVICE_NONE) { > > + platform_data.devmode = wdtmode; > > + ipc_wdt_platform_device = > > + platform_device_register_data(NULL, > > + KBUILD_MODNAME "_wdt", > > PLATFORM_DEVID_NONE, > > + &platform_data, > > + sizeof(struct > > simatic_ipc_platform)); > > + if (IS_ERR(ipc_wdt_platform_device)) > > + return PTR_ERR(ipc_wdt_platform_device); > > + > > + pr_debug("device=%s created\n", > > + ipc_wdt_platform_device->name); > > + } > > + > > + if (ledmode == SIMATIC_IPC_DEVICE_NONE && > > + wdtmode == SIMATIC_IPC_DEVICE_NONE) { > > + pr_warn("unsupported IPC detected, station > > id=%08x\n", > > + station_id); > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > Why not use MFD here? Never had a close look at mfd to be honest. I might With the custom dmi matching on 129 being part of the header, and the p2sb unhide moving out as well ... that first driver ends up being not too valuable indeed It just identifies the box and tells subsequent drivers which one it is, which watchdog and LED path to take. Moving the knowledge of which box has which LED/watchdog into the respective drivers seems to be the better way to go. So we would end up with a LED and a watchdog driver both MODULE_ALIAS("dmi:*:svnSIEMENSAG:*"); and doing the identification with the inline dmi from that header, doing p2sb with the support to come ... possibly a "//TODO\ninline" in the meantime. So no "main platform" driver anymore, but still central platform headers. Not sure how this sounds, but i think making that change should be possible. And that is what i will try and go for in v3. regards, Henning > ... > > > +/* > > + * Get membase address from PCI, used in leds and wdt modul. Here > > we read > > + * the bar0. The final address calculation is done in the > > appropriate modules > > + */ > > No blank line here. > > I would add FIXME or REVISIT here to point out that this should be > deduplicated in the future. > > > +u32 simatic_ipc_get_membase0(unsigned int p2sb) > > +{ > > + struct pci_bus *bus; > > + u32 bar0 = 0; > > + > > + /* > > + * The GPIO memory is bar0 of the hidden P2SB device. > > Unhide the device > > No, it's not a GPIO's bar. It's P2SB's one. GPIO resides in that bar > somewhere. > > > + * to have a quick look at it, before we hide it again. > > + * Also grab the pci rescan lock so that device does not > > get discovered > > + * and remapped while it is visible. > > + * This code is inspired by drivers/mfd/lpc_ich.c > > + */ > > + bus = pci_find_bus(0, 0); > > + pci_lock_rescan_remove(); > > + pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x0); > > + pci_bus_read_config_dword(bus, p2sb, PCI_BASE_ADDRESS_0, > > &bar0); + > > + bar0 &= ~0xf; > > + pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x1); > > + pci_unlock_rescan_remove(); > > + > > + return bar0; > > +} > > +EXPORT_SYMBOL(simatic_ipc_get_membase0); > > ... > > > +static inline u32 simatic_ipc_get_station_id(u8 *data, int max_len) > > +{ > > + u32 station_id = SIMATIC_IPC_INVALID_STATION_ID; > > + int i; > > Reversed xmas tree order, please. > > > + struct { > > + u8 type; /* type (0xff = binary) */ > > + u8 len; /* len of data entry */ > > + u8 reserved[3]; > > + u32 station_id; /* station id (LE) */ > > > + } __packed > > + *data_entry = (void *)data + sizeof(struct dmi_header); > > Can be one line. > > > + /* find 4th entry in OEM data */ > > + for (i = 0; i < 3; i++) > > 3 is magic! > > > + data_entry = (void *)((u8 *)(data_entry) + > > data_entry->len); + > > + /* decode station id */ > > + if (data_entry && (u8 *)data_entry < data + max_len && > > + data_entry->type == 0xff && data_entry->len == 9) > > + station_id = le32_to_cpu(data_entry->station_id); > > + > > + return station_id; > > +} >
Hi, On 3/17/21 8:13 PM, Henning Schild wrote: > Am Mon, 15 Mar 2021 12:31:11 +0200 > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > >> On Mon, Mar 15, 2021 at 12:02 PM Henning Schild >> <henning.schild@siemens.com> wrote: >>> >>> This mainly implements detection of these devices and will allow >>> secondary drivers to work on such machines. >>> >>> The identification is DMI-based with a vendor specific way to tell >>> them apart in a reliable way. >>> >>> Drivers for LEDs and Watchdogs will follow to make use of that >>> platform detection. >> >> ... >> >>> +static int register_platform_devices(u32 station_id) >>> +{ >>> + u8 ledmode = SIMATIC_IPC_DEVICE_NONE; >>> + u8 wdtmode = SIMATIC_IPC_DEVICE_NONE; >>> + int i; >>> + >>> + platform_data.devmode = SIMATIC_IPC_DEVICE_NONE; >>> + >>> + for (i = 0; i < ARRAY_SIZE(device_modes); i++) { >>> + if (device_modes[i].station_id == station_id) { >>> + ledmode = device_modes[i].led_mode; >>> + wdtmode = device_modes[i].wdt_mode; >>> + break; >>> + } >>> + } >>> + >>> + if (ledmode != SIMATIC_IPC_DEVICE_NONE) { >>> + platform_data.devmode = ledmode; >>> + ipc_led_platform_device = >>> + platform_device_register_data(NULL, >>> + KBUILD_MODNAME "_leds", >>> PLATFORM_DEVID_NONE, >>> + &platform_data, >>> + sizeof(struct >>> simatic_ipc_platform)); >>> + if (IS_ERR(ipc_led_platform_device)) >>> + return PTR_ERR(ipc_led_platform_device); >>> + >>> + pr_debug("device=%s created\n", >>> + ipc_led_platform_device->name); >>> + } >>> + >>> + if (wdtmode != SIMATIC_IPC_DEVICE_NONE) { >>> + platform_data.devmode = wdtmode; >>> + ipc_wdt_platform_device = >>> + platform_device_register_data(NULL, >>> + KBUILD_MODNAME "_wdt", >>> PLATFORM_DEVID_NONE, >>> + &platform_data, >>> + sizeof(struct >>> simatic_ipc_platform)); >>> + if (IS_ERR(ipc_wdt_platform_device)) >>> + return PTR_ERR(ipc_wdt_platform_device); >>> + >>> + pr_debug("device=%s created\n", >>> + ipc_wdt_platform_device->name); >>> + } >>> + >>> + if (ledmode == SIMATIC_IPC_DEVICE_NONE && >>> + wdtmode == SIMATIC_IPC_DEVICE_NONE) { >>> + pr_warn("unsupported IPC detected, station >>> id=%08x\n", >>> + station_id); >>> + return -EINVAL; >>> + } >>> + >>> + return 0; >>> +} >> >> Why not use MFD here? > > Never had a close look at mfd to be honest. I might > > With the custom dmi matching on 129 being part of the header, and the > p2sb unhide moving out as well ... that first driver ends up being not > too valuable indeed > > It just identifies the box and tells subsequent drivers which one it > is, which watchdog and LED path to take. Moving the knowledge of which > box has which LED/watchdog into the respective drivers seems to be the > better way to go. > > So we would end up with a LED and a watchdog driver both > MODULE_ALIAS("dmi:*:svnSIEMENSAG:*"); > and doing the identification with the inline dmi from that header, > doing p2sb with the support to come ... possibly a "//TODO\ninline" in > the meantime. > > So no "main platform" driver anymore, but still central platform > headers. > > Not sure how this sounds, but i think making that change should be > possible. And that is what i will try and go for in v3. Dropping the main drivers/platform/x86 driver sounds good to me, I was already wondering a bit about its function since it just instantiates devs to which the other ones bind to then instantiate more devs (in the LED case). Regards, Hans >> ... >> >>> +/* >>> + * Get membase address from PCI, used in leds and wdt modul. Here >>> we read >>> + * the bar0. The final address calculation is done in the >>> appropriate modules >>> + */ >> >> No blank line here. >> >> I would add FIXME or REVISIT here to point out that this should be >> deduplicated in the future. >> >>> +u32 simatic_ipc_get_membase0(unsigned int p2sb) >>> +{ >>> + struct pci_bus *bus; >>> + u32 bar0 = 0; >>> + >>> + /* >>> + * The GPIO memory is bar0 of the hidden P2SB device. >>> Unhide the device >> >> No, it's not a GPIO's bar. It's P2SB's one. GPIO resides in that bar >> somewhere. >> >>> + * to have a quick look at it, before we hide it again. >>> + * Also grab the pci rescan lock so that device does not >>> get discovered >>> + * and remapped while it is visible. >>> + * This code is inspired by drivers/mfd/lpc_ich.c >>> + */ >>> + bus = pci_find_bus(0, 0); >>> + pci_lock_rescan_remove(); >>> + pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x0); >>> + pci_bus_read_config_dword(bus, p2sb, PCI_BASE_ADDRESS_0, >>> &bar0); + >>> + bar0 &= ~0xf; >>> + pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x1); >>> + pci_unlock_rescan_remove(); >>> + >>> + return bar0; >>> +} >>> +EXPORT_SYMBOL(simatic_ipc_get_membase0); >> >> ... >> >>> +static inline u32 simatic_ipc_get_station_id(u8 *data, int max_len) >>> +{ >>> + u32 station_id = SIMATIC_IPC_INVALID_STATION_ID; >>> + int i; >> >> Reversed xmas tree order, please. >> >>> + struct { >>> + u8 type; /* type (0xff = binary) */ >>> + u8 len; /* len of data entry */ >>> + u8 reserved[3]; >>> + u32 station_id; /* station id (LE) */ >> >>> + } __packed >>> + *data_entry = (void *)data + sizeof(struct dmi_header); >> >> Can be one line. >> >>> + /* find 4th entry in OEM data */ >>> + for (i = 0; i < 3; i++) >> >> 3 is magic! >> >>> + data_entry = (void *)((u8 *)(data_entry) + >>> data_entry->len); + >>> + /* decode station id */ >>> + if (data_entry && (u8 *)data_entry < data + max_len && >>> + data_entry->type == 0xff && data_entry->len == 9) >>> + station_id = le32_to_cpu(data_entry->station_id); >>> + >>> + return station_id; >>> +} >> >
On 17.03.21 21:03, Hans de Goede wrote: Hi, >> It just identifies the box and tells subsequent drivers which one it >> is, which watchdog and LED path to take. Moving the knowledge of which >> box has which LED/watchdog into the respective drivers seems to be the >> better way to go. >> >> So we would end up with a LED and a watchdog driver both >> MODULE_ALIAS("dmi:*:svnSIEMENSAG:*"); Uh, isn't that a bit too broad ? This basically implies that Siemens will never produce boards with different configurations. >> and doing the identification with the inline dmi from that header, >> doing p2sb with the support to come ... possibly a "//TODO\ninline" in >> the meantime. >> >> So no "main platform" driver anymore, but still central platform >> headers. >> >> Not sure how this sounds, but i think making that change should be >> possible. And that is what i will try and go for in v3. > > Dropping the main drivers/platform/x86 driver sounds good to me, > I was already wondering a bit about its function since it just > instantiates devs to which the other ones bind to then instantiate > more devs (in the LED case). hmm, IMHO that depends on whether the individual sub-devices can be more generic than just that specific machine. (@Hanning: could you tell us more about that ?). Another question is how they're actually probed .. only dmi or maybe also pci dev ? (i've seen some refs to pci stuff in the led driver, but missed the other code thats called here). IMHO, if the whole thing lives on some PCI device (which can be probed via pci ID), and that device has the knowledge, where the LED registers actually are (eg. based on device ID, pci mmio mapping, ...) then there should be some parent driver that instantiates the led devices (and possibly other board specific stuff). That would be a clear separation, modularization. In that case, maybe this LED driver could even be replaced by some really generic "register-based-LED" driver, which just needs to be fed with some parameters like register ranges, bitmasks, etc. OTOH, if everything can be derived entirely from DMI match, w/o things like pci mappings involved (IOW: behaves like directly wired to the cpu's mem/io bus, no other "intelligent" bus involved), and it's all really board specific logic (no generic led or gpio controllers involved), then it might be better to have entirely separate drivers. -mtx
Hi, On 3/18/21 12:30 PM, Enrico Weigelt, metux IT consult wrote: > On 17.03.21 21:03, Hans de Goede wrote: > > Hi, > >>> It just identifies the box and tells subsequent drivers which one it >>> is, which watchdog and LED path to take. Moving the knowledge of which >>> box has which LED/watchdog into the respective drivers seems to be the >>> better way to go. >>> >>> So we would end up with a LED and a watchdog driver both >>> MODULE_ALIAS("dmi:*:svnSIEMENSAG:*"); > > Uh, isn't that a bit too broad ? This basically implies that Siemens > will never produce boards with different configurations. There is a further check done in probe() based on some Siemens specific DMI table entries. >>> and doing the identification with the inline dmi from that header, >>> doing p2sb with the support to come ... possibly a "//TODO\ninline" in >>> the meantime. >>> >>> So no "main platform" driver anymore, but still central platform >>> headers. >>> >>> Not sure how this sounds, but i think making that change should be >>> possible. And that is what i will try and go for in v3. >> >> Dropping the main drivers/platform/x86 driver sounds good to me, >> I was already wondering a bit about its function since it just >> instantiates devs to which the other ones bind to then instantiate >> more devs (in the LED case). > > hmm, IMHO that depends on whether the individual sub-devices can be > more generic than just that specific machine. (@Hanning: could you > tell us more about that ?). > > Another question is how they're actually probed .. only dmi or maybe > also pci dev ? (i've seen some refs to pci stuff in the led driver, but > missed the other code thats called here). > > IMHO, if the whole thing lives on some PCI device (which can be probed > via pci ID), and that device has the knowledge, where the LED registers > actually are (eg. based on device ID, pci mmio mapping, ...) then there > should be some parent driver that instantiates the led devices (and > possibly other board specific stuff). That would be a clear separation, > modularization. In that case, maybe this LED driver could even be > replaced by some really generic "register-based-LED" driver, which just > needs to be fed with some parameters like register ranges, bitmasks, etc. > > OTOH, if everything can be derived entirely from DMI match, w/o things > like pci mappings involved (IOW: behaves like directly wired to the > cpu's mem/io bus, no other "intelligent" bus involved), and it's all > really board specific logic (no generic led or gpio controllers > involved), then it might be better to have entirely separate drivers. FWIW I'm fine with either solution, and if we go the "parent driver" route I'm happy to have that driver sit in drivers/platform/x86 (once all the discussions surrounding this are resolved). My reply was because I noticed that the Led driver seemed to sort of also act as a parent driver (last time I looked) and instantiated a bunch of stuff, so then we have 2 parent(ish) drivers. If things stay that way then having 2 levels of parent drivers seems a bit too much to me, esp. if it can all be done cleanly in e.g. the LED driver. But as said I'm fine either way as long as the code is reasonably clean and dealing with this sort of platform specific warts happens a lot in drivers/platform/x86 . Regards, Hans
Am Thu, 18 Mar 2021 12:45:01 +0100 schrieb Hans de Goede <hdegoede@redhat.com>: > Hi, > > On 3/18/21 12:30 PM, Enrico Weigelt, metux IT consult wrote: > > On 17.03.21 21:03, Hans de Goede wrote: > > > > Hi, > > > >>> It just identifies the box and tells subsequent drivers which one > >>> it is, which watchdog and LED path to take. Moving the knowledge > >>> of which box has which LED/watchdog into the respective drivers > >>> seems to be the better way to go. > >>> > >>> So we would end up with a LED and a watchdog driver both > >>> MODULE_ALIAS("dmi:*:svnSIEMENSAG:*"); > > > > Uh, isn't that a bit too broad ? This basically implies that Siemens > > will never produce boards with different configurations. > > There is a further check done in probe() based on some Siemens > specific DMI table entries. > > >>> and doing the identification with the inline dmi from that header, > >>> doing p2sb with the support to come ... possibly a > >>> "//TODO\ninline" in the meantime. > >>> > >>> So no "main platform" driver anymore, but still central platform > >>> headers. > >>> > >>> Not sure how this sounds, but i think making that change should be > >>> possible. And that is what i will try and go for in v3. > >> > >> Dropping the main drivers/platform/x86 driver sounds good to me, > >> I was already wondering a bit about its function since it just > >> instantiates devs to which the other ones bind to then instantiate > >> more devs (in the LED case). > > > > hmm, IMHO that depends on whether the individual sub-devices can be > > more generic than just that specific machine. (@Hanning: could you > > tell us more about that ?). > > > > Another question is how they're actually probed .. only dmi or maybe > > also pci dev ? (i've seen some refs to pci stuff in the led driver, > > but missed the other code thats called here). > > > > IMHO, if the whole thing lives on some PCI device (which can be > > probed via pci ID), and that device has the knowledge, where the > > LED registers actually are (eg. based on device ID, pci mmio > > mapping, ...) then there should be some parent driver that > > instantiates the led devices (and possibly other board specific > > stuff). That would be a clear separation, modularization. In that > > case, maybe this LED driver could even be replaced by some really > > generic "register-based-LED" driver, which just needs to be fed > > with some parameters like register ranges, bitmasks, etc. > > > > OTOH, if everything can be derived entirely from DMI match, w/o > > things like pci mappings involved (IOW: behaves like directly wired > > to the cpu's mem/io bus, no other "intelligent" bus involved), and > > it's all really board specific logic (no generic led or gpio > > controllers involved), then it might be better to have entirely > > separate drivers. In fact it does dmi and not "common" but unfortunately vendor-specific. On top it does pci, so it might be fair to call it "intelligent" and keep it. > FWIW I'm fine with either solution, and if we go the "parent driver" > route I'm happy to have that driver sit in drivers/platform/x86 > (once all the discussions surrounding this are resolved). > > My reply was because I noticed that the Led driver seemed to sort of > also act as a parent driver (last time I looked) and instantiated > a bunch of stuff, so then we have 2 parent(ish) drivers. If things > stay that way then having 2 levels of parent drivers seems a bit too > much to me, esp. if it can all be done cleanly in e.g. the LED driver. One "leds" driver doing multiple leds seems to be a common pattern. So that "1 parent N children" maybe does not count as parentish. > But as said I'm fine either way as long as the code is reasonably > clean and dealing with this sort of platform specific warts happens > a lot in drivers/platform/x86 . I thought about it again and also prefer the "parent driver" idea as it is. That parent identifies the machine and depending on it, causes device drivers to be loaded. At the moment LED and watchdog, but with nvram, hwmon to come. I will stick with "platform" instead of "mfd" because it is really a machine having multiple devices. Not a device having multiple functions. regards, Henning > Regards, > > Hans >
Hi, On 3/26/21 10:55 AM, Henning Schild wrote: > Am Thu, 18 Mar 2021 12:45:01 +0100 > schrieb Hans de Goede <hdegoede@redhat.com>: > >> Hi, >> >> On 3/18/21 12:30 PM, Enrico Weigelt, metux IT consult wrote: >>> On 17.03.21 21:03, Hans de Goede wrote: >>> >>> Hi, >>> >>>>> It just identifies the box and tells subsequent drivers which one >>>>> it is, which watchdog and LED path to take. Moving the knowledge >>>>> of which box has which LED/watchdog into the respective drivers >>>>> seems to be the better way to go. >>>>> >>>>> So we would end up with a LED and a watchdog driver both >>>>> MODULE_ALIAS("dmi:*:svnSIEMENSAG:*"); >>> >>> Uh, isn't that a bit too broad ? This basically implies that Siemens >>> will never produce boards with different configurations. >> >> There is a further check done in probe() based on some Siemens >> specific DMI table entries. >> >>>>> and doing the identification with the inline dmi from that header, >>>>> doing p2sb with the support to come ... possibly a >>>>> "//TODO\ninline" in the meantime. >>>>> >>>>> So no "main platform" driver anymore, but still central platform >>>>> headers. >>>>> >>>>> Not sure how this sounds, but i think making that change should be >>>>> possible. And that is what i will try and go for in v3. >>>> >>>> Dropping the main drivers/platform/x86 driver sounds good to me, >>>> I was already wondering a bit about its function since it just >>>> instantiates devs to which the other ones bind to then instantiate >>>> more devs (in the LED case). >>> >>> hmm, IMHO that depends on whether the individual sub-devices can be >>> more generic than just that specific machine. (@Hanning: could you >>> tell us more about that ?). >>> >>> Another question is how they're actually probed .. only dmi or maybe >>> also pci dev ? (i've seen some refs to pci stuff in the led driver, >>> but missed the other code thats called here). >>> >>> IMHO, if the whole thing lives on some PCI device (which can be >>> probed via pci ID), and that device has the knowledge, where the >>> LED registers actually are (eg. based on device ID, pci mmio >>> mapping, ...) then there should be some parent driver that >>> instantiates the led devices (and possibly other board specific >>> stuff). That would be a clear separation, modularization. In that >>> case, maybe this LED driver could even be replaced by some really >>> generic "register-based-LED" driver, which just needs to be fed >>> with some parameters like register ranges, bitmasks, etc. >>> >>> OTOH, if everything can be derived entirely from DMI match, w/o >>> things like pci mappings involved (IOW: behaves like directly wired >>> to the cpu's mem/io bus, no other "intelligent" bus involved), and >>> it's all really board specific logic (no generic led or gpio >>> controllers involved), then it might be better to have entirely >>> separate drivers. > > In fact it does dmi and not "common" but unfortunately vendor-specific. > On top it does pci, so it might be fair to call it "intelligent" and > keep it. > >> FWIW I'm fine with either solution, and if we go the "parent driver" >> route I'm happy to have that driver sit in drivers/platform/x86 >> (once all the discussions surrounding this are resolved). >> >> My reply was because I noticed that the Led driver seemed to sort of >> also act as a parent driver (last time I looked) and instantiated >> a bunch of stuff, so then we have 2 parent(ish) drivers. If things >> stay that way then having 2 levels of parent drivers seems a bit too >> much to me, esp. if it can all be done cleanly in e.g. the LED driver. > > One "leds" driver doing multiple leds seems to be a common pattern. So > that "1 parent N children" maybe does not count as parentish. > >> But as said I'm fine either way as long as the code is reasonably >> clean and dealing with this sort of platform specific warts happens >> a lot in drivers/platform/x86 . > > I thought about it again and also prefer the "parent driver" idea as it > is. That parent identifies the machine and depending on it, causes > device drivers to be loaded. At the moment LED and watchdog, but with > nvram, hwmon to come. > > I will stick with "platform" instead of "mfd" because it is really a > machine having multiple devices. Not a device having multiple functions. Ok, sticking with the current separate "platform" parent driver design is fine by me. Regards, Hans
Am Mon, 15 Mar 2021 12:55:13 +0200 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > On Mon, Mar 15, 2021 at 12:12 PM Henning Schild > <henning.schild@siemens.com> wrote: > > > > 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. > > Thanks for an update! > > I did review more thoughtfully the series and realized that you can > avoid that P2SB thingy and may the approach be much cleaner if you > register the real GPIO driver and convert your LEDs to be a GPIO LEDs. > Then you won't need any custom code for it (except some board file, or > what would be better to file _DSD in your ACPI tables. For the next generation of these machines i managed to involve the BIOS guys. Goal would be to describe as much as possible in a generic and standard way in ACPI, to reduce cost on driver dev and maint in the long run. Hopefully across OSs. The first thing we wanted to look into is LEDs. The way they can be described for leds-gpio does not seem to be standard but at least seems generic. At the same time we contemplated whether to model the LEDs using the multicolor class. One thing that seems to speak against using multicolor seems to be missing ACPI "support", while regular LEDs can be described in ACPI, it does not seem like multicolor can. Or did we miss something? regards, Henning > -- > With Best Regards, > Andy Shevchenko
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 | 210 +++++++++++++++++ drivers/platform/x86/Kconfig | 12 + drivers/platform/x86/Makefile | 3 + drivers/platform/x86/pmc_atom.c | 47 ++-- drivers/platform/x86/simatic-ipc.c | 168 ++++++++++++++ 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 | 66 ++++++ 14 files changed, 761 insertions(+), 20 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