| Message ID | 20210315095710.7140-2-henning.schild@siemens.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | add device drivers for Siemens Industrial PCs | expand |
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? ... > +/* > + * 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; > +}
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? > > ... > > > +/* > > + * 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. Sure i forgot the mention that ordering problem of the two series here again specifically. Was kind of assuming yours would maybe be first and that code not being reviewed again ... The code is there to test and propose something "working" not something i expect to be merged as is. regards, Henning > > +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; > > +} >
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
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index ad4e630e73e2..44f8e82e1fd9 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -1284,6 +1284,18 @@ 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 watchdog. + + To compile this driver as a module, choose M here: the module + will be called simatic-ipc. + 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..7c32c12ad32d --- /dev/null +++ b/drivers/platform/x86/simatic-ipc.c @@ -0,0 +1,168 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Siemens SIMATIC IPC platform driver + * + * 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> + */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <linux/dmi.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/pci.h> +#include <linux/platform_data/x86/simatic-ipc.h> +#include <linux/platform_device.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) +{ + 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; +} + +/* + * 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) +{ + struct pci_bus *bus; + u32 bar0 = 0; + + /* + * 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(); + + 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("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 v2"); +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..62d2bc774067 --- /dev/null +++ b/include/linux/platform_data/x86/simatic-ipc-base.h @@ -0,0 +1,29 @@ +/* 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> + */ + +#ifndef __PLATFORM_DATA_X86_SIMATIC_IPC_BASE_H +#define __PLATFORM_DATA_X86_SIMATIC_IPC_BASE_H + +#include <linux/types.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..3af84a84f103 --- /dev/null +++ b/include/linux/platform_data/x86/simatic-ipc.h @@ -0,0 +1,66 @@ +/* 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> + */ + +#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 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, int max_len) +{ + 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 + sizeof(struct dmi_header); + + /* 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 && (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; +} + +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, dh->length); +} + +#endif /* __PLATFORM_DATA_X86_SIMATIC_IPC_H */
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: Henning Schild <henning.schild@siemens.com> --- drivers/platform/x86/Kconfig | 12 ++ drivers/platform/x86/Makefile | 3 + drivers/platform/x86/simatic-ipc.c | 168 ++++++++++++++++++ .../platform_data/x86/simatic-ipc-base.h | 29 +++ include/linux/platform_data/x86/simatic-ipc.h | 66 +++++++ 5 files changed, 278 insertions(+) create mode 100644 drivers/platform/x86/simatic-ipc.c create mode 100644 include/linux/platform_data/x86/simatic-ipc-base.h create mode 100644 include/linux/platform_data/x86/simatic-ipc.h