| Message ID | 20210302163309.25528-2-henning.schild@siemens.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | add device drivers for Siemens Industrial PCs | expand |
On Thu, Mar 4, 2021 at 8:36 AM Henning Schild <henning.schild@siemens.com> wrote: > > From: Henning Schild <henning.schild@siemens.com> > > 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. > Signed-off-by: Gerd Haeussler <gerd.haeussler.ext@siemens.com> > Signed-off-by: Henning Schild <henning.schild@siemens.com> The order is wrong taking into account the From line in the body. So, it's not clear who is the author, who is a co-developer, and who is the committer (one person may utilize few roles). Check for the rest of the series as well (basically this is the rule of thumb to recheck entire code for the comment you have got at any single place of it). ... > +config SIEMENS_SIMATIC_IPC > + tristate "Siemens Simatic IPC Class driver" > + depends on PCI > + help > + This Simatic IPC class driver is the central of several drivers. It > + is mainly used for system identification, after which drivers in other > + classes will take care of driving specifics of those machines. > + i.e. leds and watchdogs LEDs watchdog. (missed period and singular form) Module name? > endif # X86_PLATFORM_DEVICES Not sure about the ordering of the section in Kconfig, but it is up to maintainers. ... > +# Siemens Simatic Industrial PCs > +obj-$(CONFIG_SIEMENS_SIMATIC_IPC) += simatic-ipc.o Ditto. ... > + * Siemens SIMATIC IPC driver for LEDs It seems to be used in patch 4 which is not LED related. Also, why is it here if it's for LEDs? ... > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. Replace with SPDX ... > +#include <linux/module.h> > +#include <linux/kernel.h> > +#include <linux/platform_device.h> Ordered? > +#include <linux/platform_data/x86/simatic-ipc.h> ... > +static int register_platform_devices(u32 station_id) > +{ > + int i; > + u8 ledmode = SIMATIC_IPC_DEVICE_NONE; > + u8 wdtmode = SIMATIC_IPC_DEVICE_NONE; Reversed xmas tree order? > + 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)); Strange indentation (second line). > + if (IS_ERR(ipc_led_platform_device)) > + return PTR_ERR(ipc_led_platform_device); > + pr_debug(KBUILD_MODNAME ": device=%s created\n", > + ipc_led_platform_device->name); Utilize pr_fmt() instead of adding prefixes like this. > + } > + 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(KBUILD_MODNAME ": device=%s created\n", > + ipc_wdt_platform_device->name); > + } Same comments as above. > + if (ledmode == SIMATIC_IPC_DEVICE_NONE && > + wdtmode == SIMATIC_IPC_DEVICE_NONE) { > + pr_warn(KBUILD_MODNAME > + ": unsupported IPC detected, station id=%08x\n", > + station_id); Ugly indentation. With pr_fmt() in use will be much better. > + return -EINVAL; > + } else { Redundant. > + return 0; > + } > +} ... > +/* > + * 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 bar -> BAR Missed period. > + */ > + Unneeded blank line. > +u32 simatic_ipc_get_membase0(unsigned int p2sb) > +{ > + u32 bar0 = 0; > +#ifdef CONFIG_PCI It's ugly besides the fact that you have a dependency. > + struct pci_bus *bus; Missed blank line. > + /* > + * The GPIO memory is bar0 of the hidden P2SB device. Unhide the device > + * 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(); > +#endif /* CONFIG_PCI */ > + return bar0; > +} > +EXPORT_SYMBOL(simatic_ipc_get_membase0); Oy vey! I know what this is and let's do it differently. I have some (relatively old) patch series I can send you privately for testing. ... > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. Not needed when you have SPDX. ... > +#include <linux/pci.h> Wrong header. Should be types.h ... > +#include <linux/dmi.h> > +#include <linux/platform_data/x86/simatic-ipc-base.h> Missed headers. You need to include ones that the code below is a direct user of. Like types.h
Hi, On 3/4/21 11:11 AM, Andy Shevchenko wrote: > On Thu, Mar 4, 2021 at 8:36 AM Henning Schild > <henning.schild@siemens.com> wrote: >> >> From: Henning Schild <henning.schild@siemens.com> >> >> 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. > >> Signed-off-by: Gerd Haeussler <gerd.haeussler.ext@siemens.com> >> Signed-off-by: Henning Schild <henning.schild@siemens.com> > > The order is wrong taking into account the From line in the body. So, > it's not clear who is the author, who is a co-developer, and who is > the committer (one person may utilize few roles). > Check for the rest of the series as well (basically this is the rule > of thumb to recheck entire code for the comment you have got at any > single place of it). > > ... > >> +config SIEMENS_SIMATIC_IPC >> + tristate "Siemens Simatic IPC Class driver" >> + depends on PCI >> + help >> + This Simatic IPC class driver is the central of several drivers. It >> + is mainly used for system identification, after which drivers in other >> + classes will take care of driving specifics of those machines. >> + i.e. leds and watchdogs > > LEDs > watchdog. (missed period and singular form) > > Module name? > >> endif # X86_PLATFORM_DEVICES > > Not sure about the ordering of the section in Kconfig, but it is up to > maintainers. > > ... > >> +# Siemens Simatic Industrial PCs >> +obj-$(CONFIG_SIEMENS_SIMATIC_IPC) += simatic-ipc.o > > Ditto. > > ... > >> + * Siemens SIMATIC IPC driver for LEDs > > It seems to be used in patch 4 which is not LED related. Also, why is > it here if it's for LEDs? > > ... > >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. > > Replace with SPDX > > ... > >> +#include <linux/module.h> >> +#include <linux/kernel.h> >> +#include <linux/platform_device.h> > > Ordered? > >> +#include <linux/platform_data/x86/simatic-ipc.h> > > ... > >> +static int register_platform_devices(u32 station_id) >> +{ >> + int i; >> + u8 ledmode = SIMATIC_IPC_DEVICE_NONE; >> + u8 wdtmode = SIMATIC_IPC_DEVICE_NONE; > > Reversed xmas tree order? > >> + 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)); > > Strange indentation (second line). > >> + if (IS_ERR(ipc_led_platform_device)) >> + return PTR_ERR(ipc_led_platform_device); > >> + pr_debug(KBUILD_MODNAME ": device=%s created\n", >> + ipc_led_platform_device->name); > > Utilize pr_fmt() instead of adding prefixes like this. > >> + } > >> + 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(KBUILD_MODNAME ": device=%s created\n", >> + ipc_wdt_platform_device->name); >> + } > > Same comments as above. > >> + if (ledmode == SIMATIC_IPC_DEVICE_NONE && >> + wdtmode == SIMATIC_IPC_DEVICE_NONE) { >> + pr_warn(KBUILD_MODNAME >> + ": unsupported IPC detected, station id=%08x\n", >> + station_id); > > Ugly indentation. With pr_fmt() in use will be much better. > >> + return -EINVAL; > >> + } else { > > Redundant. > >> + return 0; >> + } >> +} > > ... > >> +/* >> + * 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 > > bar -> BAR > Missed period. > >> + */ > >> + > > Unneeded blank line. > >> +u32 simatic_ipc_get_membase0(unsigned int p2sb) >> +{ >> + u32 bar0 = 0; > >> +#ifdef CONFIG_PCI > > It's ugly besides the fact that you have a dependency. > >> + struct pci_bus *bus; > > Missed blank line. > >> + /* >> + * The GPIO memory is bar0 of the hidden P2SB device. Unhide the device >> + * 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(); >> +#endif /* CONFIG_PCI */ >> + return bar0; >> +} >> +EXPORT_SYMBOL(simatic_ipc_get_membase0); > > Oy vey! I know what this is and let's do it differently. I have some > (relatively old) patch series I can send you privately for testing. This bit stood out the most to me too, it would be good if we can this fixed in some cleaner work. So I'm curious how things will look with Andy's work integrated. Also I don't think this should be exported. Instead this (or its replacement) should be used to get the address for an IOMEM resource to add the platform devices when they are instantiated. Then the platform-dev drivers can just use the regular functions to get their resources instead of relying on this module. Regards, Hans > > ... > >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. > > Not needed when you have SPDX. > > ... > >> +#include <linux/pci.h> > > Wrong header. Should be types.h > > ... > >> +#include <linux/dmi.h> >> +#include <linux/platform_data/x86/simatic-ipc-base.h> > > Missed headers. You need to include ones that the code below is a > direct user of. > > Like types.h >
Hi, On 3/2/21 5:33 PM, Henning Schild wrote: <snip> > +static inline u32 simatic_ipc_get_station_id(u8 *data) > +{ > + u32 station_id = SIMATIC_IPC_INVALID_STATION_ID; > + int i; > + 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; > + > + /* find 4th entry in OEM data */ > + for (i = 0; i < 3; i++) > + data_entry = (void *)((u8 *)(data_entry) + data_entry->len); > + > + /* decode station id */ > + if (data_entry && data_entry->type == 0xff && data_entry->len == 9) > + station_id = le32_to_cpu(data_entry->station_id); > + > + return station_id; > +} > + > +static inline void > +simatic_ipc_find_dmi_entry_helper(const struct dmi_header *dh, void *_data) > +{ > + u32 *id = _data; > + > + if (dh->type != DMI_ENTRY_OEM) > + return; > + > + *id = simatic_ipc_get_station_id((u8 *)dh + sizeof(struct dmi_header)); > +} Please take dh->length into account here and make sure that you don't walk past the end of the DMI tables during the parsing here. Regards, Hans > + > +#endif /* __PLATFORM_DATA_X86_SIMATIC_IPC_H */ >
Am Thu, 4 Mar 2021 12:11:12 +0200 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > On Thu, Mar 4, 2021 at 8:36 AM Henning Schild > <henning.schild@siemens.com> wrote: > > > > From: Henning Schild <henning.schild@siemens.com> > > > > 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. > > > Signed-off-by: Gerd Haeussler <gerd.haeussler.ext@siemens.com> > > Signed-off-by: Henning Schild <henning.schild@siemens.com> > > The order is wrong taking into account the From line in the body. So, > it's not clear who is the author, who is a co-developer, and who is > the committer (one person may utilize few roles). > Check for the rest of the series as well (basically this is the rule > of thumb to recheck entire code for the comment you have got at any > single place of it). For some code Gerd is the author, and i am the co-Author. We even have Jan in the mix at places. At least in copyright headers. I will remain the committer for the three of us. And since i do not know exactly what the problem is i will add only my Signed-off to avoid confusion. Please speak up it that would be wrong as well and point me to the docs i missed. Or tell me how it should be done. > ... > > > +config SIEMENS_SIMATIC_IPC > > + tristate "Siemens Simatic IPC Class driver" > > + depends on PCI > > + help > > + This Simatic IPC class driver is the central of several > > drivers. It > > + is mainly used for system identification, after which > > drivers in other > > + classes will take care of driving specifics of those > > machines. > > + i.e. leds and watchdogs > > LEDs > watchdog. (missed period and singular form) > > Module name? > > > endif # X86_PLATFORM_DEVICES > > Not sure about the ordering of the section in Kconfig, but it is up to > maintainers. > > ... > > > +# Siemens Simatic Industrial PCs > > +obj-$(CONFIG_SIEMENS_SIMATIC_IPC) += simatic-ipc.o > > Ditto. I will check both again if a find a pattern, otherwise will wait for maintainers to complain and hopefully suggest. > ... > > > + * Siemens SIMATIC IPC driver for LEDs > > It seems to be used in patch 4 which is not LED related. Also, why is > it here if it's for LEDs? > > ... > > > + * This program is free software; you can redistribute it and/or > > modify > > + * it under the terms of the GNU General Public License version 2 > > as > > + * published by the Free Software Foundation. > > Replace with SPDX > > ... > > > +#include <linux/module.h> > > +#include <linux/kernel.h> > > +#include <linux/platform_device.h> > > Ordered? > > > +#include <linux/platform_data/x86/simatic-ipc.h> > > ... > > > +static int register_platform_devices(u32 station_id) > > +{ > > + int i; > > + u8 ledmode = SIMATIC_IPC_DEVICE_NONE; > > + u8 wdtmode = SIMATIC_IPC_DEVICE_NONE; > > Reversed xmas tree order? I do not get this, it is almost easter egg order time. Please explain. > > + 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)); > > Strange indentation (second line). > > > + if (IS_ERR(ipc_led_platform_device)) > > + return PTR_ERR(ipc_led_platform_device); > > > + pr_debug(KBUILD_MODNAME ": device=%s created\n", > > + ipc_led_platform_device->name); > > Utilize pr_fmt() instead of adding prefixes like this. > > > + } > > > + 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(KBUILD_MODNAME ": device=%s created\n", > > + ipc_wdt_platform_device->name); > > + } > > Same comments as above. > > > + if (ledmode == SIMATIC_IPC_DEVICE_NONE && > > + wdtmode == SIMATIC_IPC_DEVICE_NONE) { > > + pr_warn(KBUILD_MODNAME > > + ": unsupported IPC detected, station > > id=%08x\n", > > + station_id); > > Ugly indentation. With pr_fmt() in use will be much better. > > > + return -EINVAL; > > > + } else { > > Redundant. > > > + return 0; > > + } > > +} > > ... > > > +/* > > + * 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 > > bar -> BAR > Missed period. > > > + */ > > > + > > Unneeded blank line. > > > +u32 simatic_ipc_get_membase0(unsigned int p2sb) > > +{ > > + u32 bar0 = 0; > > > +#ifdef CONFIG_PCI > > It's ugly besides the fact that you have a dependency. left over from out-of-tree, will be removed rest is clear, Thanks! Henning > > + struct pci_bus *bus; > > Missed blank line. > > > + /* > > + * The GPIO memory is bar0 of the hidden P2SB device. > > Unhide the device > > + * 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(); > > +#endif /* CONFIG_PCI */ > > + return bar0; > > +} > > +EXPORT_SYMBOL(simatic_ipc_get_membase0); > > Oy vey! I know what this is and let's do it differently. I have some > (relatively old) patch series I can send you privately for testing. > > ... > > > + * This program is free software; you can redistribute it and/or > > modify > > + * it under the terms of the GNU General Public License version 2 > > as > > + * published by the Free Software Foundation. > > Not needed when you have SPDX. > > ... > > > +#include <linux/pci.h> > > Wrong header. Should be types.h > > ... > > > +#include <linux/dmi.h> > > +#include <linux/platform_data/x86/simatic-ipc-base.h> > > Missed headers. You need to include ones that the code below is a > direct user of. > > Like types.h >
On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 3/4/21 11:11 AM, Andy Shevchenko wrote: > > On Thu, Mar 4, 2021 at 8:36 AM Henning Schild > > <henning.schild@siemens.com> wrote: ... > >> +u32 simatic_ipc_get_membase0(unsigned int p2sb) > >> +{ > >> + u32 bar0 = 0; > > > >> +#ifdef CONFIG_PCI > > > > It's ugly besides the fact that you have a dependency. > > > >> + struct pci_bus *bus; > > > > Missed blank line. > > > >> + /* > >> + * The GPIO memory is bar0 of the hidden P2SB device. Unhide the device > >> + * 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(); > >> +#endif /* CONFIG_PCI */ > >> + return bar0; > >> +} > >> +EXPORT_SYMBOL(simatic_ipc_get_membase0); > > > > Oy vey! I know what this is and let's do it differently. I have some > > (relatively old) patch series I can send you privately for testing. > > This bit stood out the most to me too, it would be good if we can this fixed > in some cleaner work. So I'm curious how things will look with Andy's work > integrated. > > Also I don't think this should be exported. Instead this (or its replacement) > should be used to get the address for an IOMEM resource to add the platform > devices when they are instantiated. Then the platform-dev drivers can just > use the regular functions to get their resources instead of relying on this > module. I have published a WIP branch [1]. I have no means to test (I don't know what hardware at hand I can use right now), but I made it compile after 4 years of gathering dust... Feel free to give any kind of comments or share your ideas on how it can be improved (the above idea on IOMEM resource is interesting, but devices are PCI, not sure how this can be done). [1]: https://gitlab.com/andy-shev/next/-/tree/p2sb
On Thu, Mar 4, 2021 at 9:52 PM Henning Schild <henning.schild@siemens.com> wrote: > Am Thu, 4 Mar 2021 12:11:12 +0200 > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > On Thu, Mar 4, 2021 at 8:36 AM Henning Schild > > <henning.schild@siemens.com> wrote: ... > > Check for the rest of the series as well (basically this is the rule > > of thumb to recheck entire code for the comment you have got at any > > single place of it). > > For some code Gerd is the author, and i am the co-Author. We even have > Jan in the mix at places. At least in copyright headers. > > I will remain the committer for the three of us. And since i do not > know exactly what the problem is i will add only my Signed-off to avoid > confusion. > > Please speak up it that would be wrong as well and point me to the docs > i missed. Or tell me how it should be done. Then make sure that you have From line with the Author (`git commit --amend --author="..."`) and add your Co-developed-by tag. ... > > > + int i; > > > + u8 ledmode = SIMATIC_IPC_DEVICE_NONE; > > > + u8 wdtmode = SIMATIC_IPC_DEVICE_NONE; > > > > Reversed xmas tree order? > > I do not get this, it is almost easter egg order time. Please explain. Longer lines usually go first. See above :-)
Hi, On 3/5/21 4:42 PM, Andy Shevchenko wrote: > On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede <hdegoede@redhat.com> wrote: >> On 3/4/21 11:11 AM, Andy Shevchenko wrote: >>> On Thu, Mar 4, 2021 at 8:36 AM Henning Schild >>> <henning.schild@siemens.com> wrote: > > ... > >>>> +u32 simatic_ipc_get_membase0(unsigned int p2sb) >>>> +{ >>>> + u32 bar0 = 0; >>> >>>> +#ifdef CONFIG_PCI >>> >>> It's ugly besides the fact that you have a dependency. >>> >>>> + struct pci_bus *bus; >>> >>> Missed blank line. >>> >>>> + /* >>>> + * The GPIO memory is bar0 of the hidden P2SB device. Unhide the device >>>> + * 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(); >>>> +#endif /* CONFIG_PCI */ >>>> + return bar0; >>>> +} >>>> +EXPORT_SYMBOL(simatic_ipc_get_membase0); >>> >>> Oy vey! I know what this is and let's do it differently. I have some >>> (relatively old) patch series I can send you privately for testing. >> >> This bit stood out the most to me too, it would be good if we can this fixed >> in some cleaner work. So I'm curious how things will look with Andy's work >> integrated. >> >> Also I don't think this should be exported. Instead this (or its replacement) >> should be used to get the address for an IOMEM resource to add the platform >> devices when they are instantiated. Then the platform-dev drivers can just >> use the regular functions to get their resources instead of relying on this >> module. > > I have published a WIP branch [1]. I have no means to test (I don't > know what hardware at hand I can use right now), but I made it compile > after 4 years of gathering dust... So I took a quick look at the following 2 commits: "platform/x86: p2sb: New Primary to Sideband bridge support library" "mfd: lpc_ich: Switch to generic p2sb_bar()" And this looks good to me, although compared to the code from this patch-set you are missing the pci_lock_rescan_remove(); and pci_unlock_rescan_remove(); calls. > Feel free to give any kind of comments or share your ideas on how it > can be improved (the above idea on IOMEM resource is interesting, but > devices are PCI, not sure how this can be done). The code added by this patch introduces a register_platform_devices() function which creates a bunch of platform-devices; and then the device-drivers for those call simatic_ipc_get_membase0() to get their base-address. My suggestion was to instead put the simatic_ipc_get_membase0() call inside the code instantiating the platform devices and to add the base-address for that pdev as IOMEM resource to the instantiated platform-devices. I hope this helps to clarify what I was trying to say. Regards, Hans
On Fri, Mar 5, 2021 at 6:14 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 3/5/21 4:42 PM, Andy Shevchenko wrote: > > On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede <hdegoede@redhat.com> wrote: > >> On 3/4/21 11:11 AM, Andy Shevchenko wrote: > >>> On Thu, Mar 4, 2021 at 8:36 AM Henning Schild > >>> <henning.schild@siemens.com> wrote: ... > >>> Oy vey! I know what this is and let's do it differently. I have some > >>> (relatively old) patch series I can send you privately for testing. > >> > >> This bit stood out the most to me too, it would be good if we can this fixed > >> in some cleaner work. So I'm curious how things will look with Andy's work > >> integrated. > >> > >> Also I don't think this should be exported. Instead this (or its replacement) > >> should be used to get the address for an IOMEM resource to add the platform > >> devices when they are instantiated. Then the platform-dev drivers can just > >> use the regular functions to get their resources instead of relying on this > >> module. > > > > I have published a WIP branch [1]. I have no means to test (I don't > > know what hardware at hand I can use right now), but I made it compile > > after 4 years of gathering dust... > > So I took a quick look at the following 2 commits: (One of the latter commits moves the code to drivers/pci/pci-p2sb.c, do you think it's better like that? The idea is to deduplicate __pci_bus_read_base() call) > "platform/x86: p2sb: New Primary to Sideband bridge support library" > "mfd: lpc_ich: Switch to generic p2sb_bar()" > > And this looks good to me, although compared to the code from this > patch-set you are missing the pci_lock_rescan_remove(); and > pci_unlock_rescan_remove(); calls. Oh, indeed. > > Feel free to give any kind of comments or share your ideas on how it > > can be improved (the above idea on IOMEM resource is interesting, but > > devices are PCI, not sure how this can be done). > > The code added by this patch introduces a register_platform_devices() > function which creates a bunch of platform-devices; and then the > device-drivers for those call simatic_ipc_get_membase0() to get their > base-address. Sounds like an MFD approach... > My suggestion was to instead put the simatic_ipc_get_membase0() call > inside the code instantiating the platform devices and to add the > base-address for that pdev as IOMEM resource to the instantiated > platform-devices. > > I hope this helps to clarify what I was trying to say. Yes, thanks!
Am Fri, 5 Mar 2021 17:46:08 +0200 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > On Thu, Mar 4, 2021 at 9:52 PM Henning Schild > <henning.schild@siemens.com> wrote: > > Am Thu, 4 Mar 2021 12:11:12 +0200 > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > > On Thu, Mar 4, 2021 at 8:36 AM Henning Schild > > > <henning.schild@siemens.com> wrote: > > ... > > > > Check for the rest of the series as well (basically this is the > > > rule of thumb to recheck entire code for the comment you have got > > > at any single place of it). > > > > For some code Gerd is the author, and i am the co-Author. We even > > have Jan in the mix at places. At least in copyright headers. > > > > I will remain the committer for the three of us. And since i do not > > know exactly what the problem is i will add only my Signed-off to > > avoid confusion. > > > > Please speak up it that would be wrong as well and point me to the > > docs i missed. Or tell me how it should be done. > > Then make sure that you have From line with the Author (`git commit > --amend --author="..."`) and add your Co-developed-by tag. > > ... > > > > > + int i; > > > > + u8 ledmode = SIMATIC_IPC_DEVICE_NONE; > > > > + u8 wdtmode = SIMATIC_IPC_DEVICE_NONE; > > > > > > Reversed xmas tree order? > > > > I do not get this, it is almost easter egg order time. Please > > explain. > > Longer lines > usually go > first. Henning see i > See above :-) >
Hi, On 3/5/21 5:25 PM, Andy Shevchenko wrote: > On Fri, Mar 5, 2021 at 6:14 PM Hans de Goede <hdegoede@redhat.com> wrote: >> On 3/5/21 4:42 PM, Andy Shevchenko wrote: >>> On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede <hdegoede@redhat.com> wrote: >>>> On 3/4/21 11:11 AM, Andy Shevchenko wrote: >>>>> On Thu, Mar 4, 2021 at 8:36 AM Henning Schild >>>>> <henning.schild@siemens.com> wrote: > > ... > >>>>> Oy vey! I know what this is and let's do it differently. I have some >>>>> (relatively old) patch series I can send you privately for testing. >>>> >>>> This bit stood out the most to me too, it would be good if we can this fixed >>>> in some cleaner work. So I'm curious how things will look with Andy's work >>>> integrated. >>>> >>>> Also I don't think this should be exported. Instead this (or its replacement) >>>> should be used to get the address for an IOMEM resource to add the platform >>>> devices when they are instantiated. Then the platform-dev drivers can just >>>> use the regular functions to get their resources instead of relying on this >>>> module. >>> >>> I have published a WIP branch [1]. I have no means to test (I don't >>> know what hardware at hand I can use right now), but I made it compile >>> after 4 years of gathering dust... >> >> So I took a quick look at the following 2 commits: > > (One of the latter commits moves the code to drivers/pci/pci-p2sb.c, > do you think it's better like that? The idea is to deduplicate > __pci_bus_read_base() call) > >> "platform/x86: p2sb: New Primary to Sideband bridge support library" >> "mfd: lpc_ich: Switch to generic p2sb_bar()" >> >> And this looks good to me, although compared to the code from this >> patch-set you are missing the pci_lock_rescan_remove(); and >> pci_unlock_rescan_remove(); calls. > > Oh, indeed. > >>> Feel free to give any kind of comments or share your ideas on how it >>> can be improved (the above idea on IOMEM resource is interesting, but >>> devices are PCI, not sure how this can be done). >> >> The code added by this patch introduces a register_platform_devices() >> function which creates a bunch of platform-devices; and then the >> device-drivers for those call simatic_ipc_get_membase0() to get their >> base-address. > > Sounds like an MFD approach... Yes except that there does not seem to be a clear parent for these devices, so it is MFD-ish. IOW I'm not sure this should be changed to use the MFD framework, but I was thinking along those line myself too. Henning did you look into using the MFD framework + MFD cell descriptions to instantiate the platform-devices for you ? Regards, Hans
On Fri, Mar 5, 2021 at 6:25 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Mar 5, 2021 at 6:14 PM Hans de Goede <hdegoede@redhat.com> wrote: > > On 3/5/21 4:42 PM, Andy Shevchenko wrote: ... > > So I took a quick look at the following 2 commits: > > (One of the latter commits moves the code to drivers/pci/pci-p2sb.c, > do you think it's better like that? The idea is to deduplicate > __pci_bus_read_base() call) > > > "platform/x86: p2sb: New Primary to Sideband bridge support library" > > "mfd: lpc_ich: Switch to generic p2sb_bar()" > > > > And this looks good to me, although compared to the code from this > > patch-set you are missing the pci_lock_rescan_remove(); and > > pci_unlock_rescan_remove(); calls. > > Oh, indeed. Correction here, I'm using pci_bus_sem in the latest version.
Am Fri, 5 Mar 2021 17:42:42 +0200 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede <hdegoede@redhat.com> > wrote: > > On 3/4/21 11:11 AM, Andy Shevchenko wrote: > > > On Thu, Mar 4, 2021 at 8:36 AM Henning Schild > > > <henning.schild@siemens.com> wrote: > > ... > > > >> +u32 simatic_ipc_get_membase0(unsigned int p2sb) > > >> +{ > > >> + u32 bar0 = 0; > > > > > >> +#ifdef CONFIG_PCI > > > > > > It's ugly besides the fact that you have a dependency. > > > > > >> + struct pci_bus *bus; > > > > > > Missed blank line. > > > > > >> + /* > > >> + * The GPIO memory is bar0 of the hidden P2SB device. > > >> Unhide the device > > >> + * 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(); > > >> +#endif /* CONFIG_PCI */ > > >> + return bar0; > > >> +} > > >> +EXPORT_SYMBOL(simatic_ipc_get_membase0); > > > > > > Oy vey! I know what this is and let's do it differently. I have > > > some (relatively old) patch series I can send you privately for > > > testing. > > > > This bit stood out the most to me too, it would be good if we can > > this fixed in some cleaner work. So I'm curious how things will > > look with Andy's work integrated. > > > > Also I don't think this should be exported. Instead this (or its > > replacement) should be used to get the address for an IOMEM > > resource to add the platform devices when they are instantiated. > > Then the platform-dev drivers can just use the regular functions to > > get their resources instead of relying on this module. > > I have published a WIP branch [1]. I have no means to test (I don't > know what hardware at hand I can use right now), but I made it compile > after 4 years of gathering dust... > Feel free to give any kind of comments or share your ideas on how it > can be improved (the above idea on IOMEM resource is interesting, but > devices are PCI, not sure how this can be done). > > [1]: https://gitlab.com/andy-shev/next/-/tree/p2sb That is a little weird, might be a good idea to RFC reply to the cover letter of this one. To allow review and discussion in a central place. Henning
On Fri, Mar 5, 2021 at 6:41 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Mar 5, 2021 at 6:25 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Fri, Mar 5, 2021 at 6:14 PM Hans de Goede <hdegoede@redhat.com> wrote: > > > On 3/5/21 4:42 PM, Andy Shevchenko wrote: > > ... > > > > So I took a quick look at the following 2 commits: > > > > (One of the latter commits moves the code to drivers/pci/pci-p2sb.c, > > do you think it's better like that? The idea is to deduplicate > > __pci_bus_read_base() call) > > > > > "platform/x86: p2sb: New Primary to Sideband bridge support library" > > > "mfd: lpc_ich: Switch to generic p2sb_bar()" > > > > > > And this looks good to me, although compared to the code from this > > > patch-set you are missing the pci_lock_rescan_remove(); and > > > pci_unlock_rescan_remove(); calls. > > > > Oh, indeed. > > Correction here, I'm using pci_bus_sem in the latest version. Okay, it seems that semaphore is not what I need and PCI rescan lock is what is mandatory to take here.
On Fri, Mar 5, 2021 at 6:47 PM Henning Schild <henning.schild@siemens.com> wrote: > Am Fri, 5 Mar 2021 17:42:42 +0200 > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede <hdegoede@redhat.com> > > wrote: ... > > [1]: https://gitlab.com/andy-shev/next/-/tree/p2sb > > That is a little weird, might be a good idea to RFC reply to the cover > letter of this one. To allow review and discussion in a central place. I'm now rebasing it to be more presentable. If you can test this approach and it works for you, I'll send a formal RFC series.
On Fri, Mar 5, 2021 at 7:17 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Mar 5, 2021 at 6:47 PM Henning Schild > <henning.schild@siemens.com> wrote: > > Am Fri, 5 Mar 2021 17:42:42 +0200 > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > > On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede <hdegoede@redhat.com> > > > wrote: > > ... > > > > [1]: https://gitlab.com/andy-shev/next/-/tree/p2sb > > > > That is a little weird, might be a good idea to RFC reply to the cover > > letter of this one. To allow review and discussion in a central place. > > I'm now rebasing it to be more presentable. > If you can test this approach and it works for you, I'll send a formal > RFC series. Okay, [1] now is in presentable shape, each patch with a proper commit message and authorship, also all patches are compiled without issues.
Am Fri, 5 Mar 2021 19:44:57 +0200 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > On Fri, Mar 5, 2021 at 7:17 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > > > On Fri, Mar 5, 2021 at 6:47 PM Henning Schild > > <henning.schild@siemens.com> wrote: > > > Am Fri, 5 Mar 2021 17:42:42 +0200 > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > > > On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede > > > > <hdegoede@redhat.com> wrote: > > > > ... > > > > > > [1]: https://gitlab.com/andy-shev/next/-/tree/p2sb > > > > > > That is a little weird, might be a good idea to RFC reply to the > > > cover letter of this one. To allow review and discussion in a > > > central place. > > > > I'm now rebasing it to be more presentable. > > If you can test this approach and it works for you, I'll send a > > formal RFC series. > > Okay, [1] now is in presentable shape, each patch with a proper commit > message and authorship, also all patches are compiled without issues. Thank you so much, i will base v2 on that and let you know how that works. regards, Henning
On Mon, Mar 8, 2021 at 3:02 PM Henning Schild <henning.schild@siemens.com> wrote: > Am Fri, 5 Mar 2021 19:44:57 +0200 > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > On Fri, Mar 5, 2021 at 7:17 PM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > On Fri, Mar 5, 2021 at 6:47 PM Henning Schild > > > <henning.schild@siemens.com> wrote: > > > > Am Fri, 5 Mar 2021 17:42:42 +0200 > > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > > > > On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede > > > > > <hdegoede@redhat.com> wrote: ... > > > > > [1]: https://gitlab.com/andy-shev/next/-/tree/p2sb > > > > > > > > That is a little weird, might be a good idea to RFC reply to the > > > > cover letter of this one. To allow review and discussion in a > > > > central place. > > > > > > I'm now rebasing it to be more presentable. > > > If you can test this approach and it works for you, I'll send a > > > formal RFC series. > > > > Okay, [1] now is in presentable shape, each patch with a proper commit > > message and authorship, also all patches are compiled without issues. > > Thank you so much, i will base v2 on that and let you know how that > works. I went ahead and submitted the series [2]. Feel free either to use the last 7 patches from [1], or the series. In either case, if it works for you I would expect the Tested-by tag given against _series_. Thanks! (Or comment there what is not working / needed for your case) [2]: https://lore.kernel.org/linux-pci/20210308122020.57071-1-andriy.shevchenko@linux.intel.com/T/#t
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index ad4e630e73e2..bb9e282d89cf 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -1284,6 +1284,15 @@ config INTEL_TELEMETRY directly via debugfs files. Various tools may use this interface for SoC state monitoring. +config SIEMENS_SIMATIC_IPC + tristate "Siemens Simatic IPC Class driver" + depends on PCI + help + This Simatic IPC class driver is the central of several drivers. It + is mainly used for system identification, after which drivers in other + classes will take care of driving specifics of those machines. + i.e. leds and watchdogs + endif # X86_PLATFORM_DEVICES config PMC_ATOM diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile index 60d554073749..26cdebf2e701 100644 --- a/drivers/platform/x86/Makefile +++ b/drivers/platform/x86/Makefile @@ -138,3 +138,6 @@ obj-$(CONFIG_INTEL_TELEMETRY) += intel_telemetry_core.o \ intel_telemetry_pltdrv.o \ intel_telemetry_debugfs.o obj-$(CONFIG_PMC_ATOM) += pmc_atom.o + +# Siemens Simatic Industrial PCs +obj-$(CONFIG_SIEMENS_SIMATIC_IPC) += simatic-ipc.o diff --git a/drivers/platform/x86/simatic-ipc.c b/drivers/platform/x86/simatic-ipc.c new file mode 100644 index 000000000000..6adcdac1a0d7 --- /dev/null +++ b/drivers/platform/x86/simatic-ipc.c @@ -0,0 +1,166 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Siemens SIMATIC IPC driver for LEDs + * + * Copyright (c) Siemens AG, 2018-2021 + * + * Authors: + * Henning Schild <henning.schild@siemens.com> + * Jan Kiszka <jan.kiszka@siemens.com> + * Gerd Haeussler <gerd.haeussler.ext@siemens.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include <linux/module.h> +#include <linux/kernel.h> +#include <linux/platform_device.h> +#include <linux/platform_data/x86/simatic-ipc.h> + +static struct platform_device *ipc_led_platform_device; +static struct platform_device *ipc_wdt_platform_device; + +static const struct dmi_system_id simatic_ipc_whitelist[] = { + { + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"), + }, + }, + {} +}; + +static struct simatic_ipc_platform platform_data; + +static struct { + u32 station_id; + u8 led_mode; + u8 wdt_mode; +} device_modes[] = { + { SIMATIC_IPC_IPC127E, SIMATIC_IPC_DEVICE_127E, SIMATIC_IPC_DEVICE_NONE}, + { SIMATIC_IPC_IPC227D, SIMATIC_IPC_DEVICE_227D, SIMATIC_IPC_DEVICE_NONE}, + { SIMATIC_IPC_IPC227E, SIMATIC_IPC_DEVICE_427E, SIMATIC_IPC_DEVICE_227E}, + { SIMATIC_IPC_IPC277E, SIMATIC_IPC_DEVICE_NONE, SIMATIC_IPC_DEVICE_227E}, + { SIMATIC_IPC_IPC427D, SIMATIC_IPC_DEVICE_427E, SIMATIC_IPC_DEVICE_NONE}, + { SIMATIC_IPC_IPC427E, SIMATIC_IPC_DEVICE_427E, SIMATIC_IPC_DEVICE_427E}, + { SIMATIC_IPC_IPC477E, SIMATIC_IPC_DEVICE_NONE, SIMATIC_IPC_DEVICE_427E}, +}; + +static int register_platform_devices(u32 station_id) +{ + int i; + u8 ledmode = SIMATIC_IPC_DEVICE_NONE; + u8 wdtmode = SIMATIC_IPC_DEVICE_NONE; + + 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(KBUILD_MODNAME ": 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(KBUILD_MODNAME ": device=%s created\n", + ipc_wdt_platform_device->name); + } + + if (ledmode == SIMATIC_IPC_DEVICE_NONE && + wdtmode == SIMATIC_IPC_DEVICE_NONE) { + pr_warn(KBUILD_MODNAME + ": unsupported IPC detected, station id=%08x\n", + station_id); + return -EINVAL; + } else { + return 0; + } +} + +/* + * 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 + */ + +u32 simatic_ipc_get_membase0(unsigned int p2sb) +{ + u32 bar0 = 0; +#ifdef CONFIG_PCI + struct pci_bus *bus; + /* + * The GPIO memory is bar0 of the hidden P2SB device. Unhide the device + * 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(); +#endif /* CONFIG_PCI */ + return bar0; +} +EXPORT_SYMBOL(simatic_ipc_get_membase0); + +static int __init simatic_ipc_init_module(void) +{ + const struct dmi_system_id *match; + u32 station_id; + int err; + + match = dmi_first_match(simatic_ipc_whitelist); + if (!match) + return 0; + + err = dmi_walk(simatic_ipc_find_dmi_entry_helper, &station_id); + + if (err || station_id == SIMATIC_IPC_INVALID_STATION_ID) { + pr_warn(KBUILD_MODNAME ": DMI entry %d not found\n", + DMI_ENTRY_OEM); + return 0; + } + + return register_platform_devices(station_id); +} + +static void __exit simatic_ipc_exit_module(void) +{ + platform_device_unregister(ipc_led_platform_device); + ipc_led_platform_device = NULL; + + platform_device_unregister(ipc_wdt_platform_device); + ipc_wdt_platform_device = NULL; +} + +module_init(simatic_ipc_init_module); +module_exit(simatic_ipc_exit_module); + +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Gerd Haeussler <gerd.haeussler.ext@siemens.com>"); +MODULE_ALIAS("dmi:*:svnSIEMENSAG:*"); diff --git a/include/linux/platform_data/x86/simatic-ipc-base.h b/include/linux/platform_data/x86/simatic-ipc-base.h new file mode 100644 index 000000000000..d8e59eb8fc96 --- /dev/null +++ b/include/linux/platform_data/x86/simatic-ipc-base.h @@ -0,0 +1,33 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Siemens SIMATIC IPC drivers + * + * Copyright (c) Siemens AG, 2018-2021 + * + * Authors: + * Henning Schild <henning.schild@siemens.com> + * Gerd Haeussler <gerd.haeussler.ext@siemens.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#ifndef __PLATFORM_DATA_X86_SIMATIC_IPC_BASE_H +#define __PLATFORM_DATA_X86_SIMATIC_IPC_BASE_H + +#include <linux/pci.h> + +#define SIMATIC_IPC_DEVICE_NONE 0 +#define SIMATIC_IPC_DEVICE_227D 1 +#define SIMATIC_IPC_DEVICE_427E 2 +#define SIMATIC_IPC_DEVICE_127E 3 +#define SIMATIC_IPC_DEVICE_227E 4 + +struct simatic_ipc_platform { + u8 devmode; +}; + +u32 simatic_ipc_get_membase0(unsigned int p2sb); + +#endif /* __PLATFORM_DATA_X86_SIMATIC_IPC_BASE_H */ diff --git a/include/linux/platform_data/x86/simatic-ipc.h b/include/linux/platform_data/x86/simatic-ipc.h new file mode 100644 index 000000000000..90bb0d7cf09a --- /dev/null +++ b/include/linux/platform_data/x86/simatic-ipc.h @@ -0,0 +1,68 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Siemens SIMATIC IPC drivers + * + * Copyright (c) Siemens AG, 2018-2021 + * + * Authors: + * Henning Schild <henning.schild@siemens.com> + * Gerd Haeussler <gerd.haeussler.ext@siemens.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#ifndef __PLATFORM_DATA_X86_SIMATIC_IPC_H +#define __PLATFORM_DATA_X86_SIMATIC_IPC_H + +#include <linux/dmi.h> +#include <linux/platform_data/x86/simatic-ipc-base.h> + +#define DMI_ENTRY_OEM 129 + +enum simatic_ipc_station_ids { + SIMATIC_IPC_INVALID_STATION_ID = 0, + SIMATIC_IPC_IPC227D = 0x00000501, + SIMATIC_IPC_IPC427D = 0x00000701, + SIMATIC_IPC_IPC227E = 0x00000901, + SIMATIC_IPC_IPC277E = 0x00000902, + SIMATIC_IPC_IPC427E = 0x00000A01, + SIMATIC_IPC_IPC477E = 0x00000A02, + SIMATIC_IPC_IPC127E = 0x00000D01, +}; + +static inline u32 simatic_ipc_get_station_id(u8 *data) +{ + u32 station_id = SIMATIC_IPC_INVALID_STATION_ID; + int i; + 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; + + /* find 4th entry in OEM data */ + for (i = 0; i < 3; i++) + data_entry = (void *)((u8 *)(data_entry) + data_entry->len); + + /* decode station id */ + if (data_entry && data_entry->type == 0xff && data_entry->len == 9) + station_id = le32_to_cpu(data_entry->station_id); + + return station_id; +} + +static inline void +simatic_ipc_find_dmi_entry_helper(const struct dmi_header *dh, void *_data) +{ + u32 *id = _data; + + if (dh->type != DMI_ENTRY_OEM) + return; + + *id = simatic_ipc_get_station_id((u8 *)dh + sizeof(struct dmi_header)); +} + +#endif /* __PLATFORM_DATA_X86_SIMATIC_IPC_H */