| Message ID | 20210302163309.25528-1-henning.schild@siemens.com |
|---|---|
| Headers | show |
| Series | add device drivers for Siemens Industrial PCs | expand |
On Thu, Mar 4, 2021 at 9:27 AM Henning Schild <henning.schild@siemens.com> wrote: > Siemens industrial PCs unfortunately can not always be properly > identified the way we used to. An earlier commit introduced code that > allows proper identification without looking at DMI strings that could > differ based on product branding. > Switch over to that proper way and revert commits that used to collect > the machines based on unstable strings. > +#include <linux/platform_data/x86/simatic-ipc.h> > +static int pmc_clk_is_critical(const struct dmi_system_id *d) > +{ > + int ret = true; > + u32 station_id; > + > + if (!strcmp(d->ident, "SIEMENS AG")) { > + if (dmi_walk(simatic_ipc_find_dmi_entry_helper, &station_id)) > + ret = false; > + else > + ret = (station_id == SIMATIC_IPC_IPC227E || > + station_id == SIMATIC_IPC_IPC277E); > + } > + > + return ret; Much easier to rewrite it as if (strcmp(...)) // BTW, do we have a dmi_* helper for that? return true; if (dmi_walk) return false; return station_id == || ...; > +} Maybe instead you can rewrite it as a callback in DMI table which changes a (global, yeah) variable that you simply reassign... > if (d) { > - clk_data->critical = true; > - pr_info("%s critclks quirk enabled\n", d->ident); > + clk_data->critical = pmc_clk_is_critical(d); > + if (clk_data->critical) > + pr_info("%s critclks quirk enabled\n", d->ident); > } ...somewhere here? Like clk_data->critical = global_var; if (...) pr_info(); It seems it will reduce burden on a callback by dropping strcmp() call. -- With Best Regards, Andy Shevchenko
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 -- With Best Regards, Andy Shevchenko
On Thu, Mar 4, 2021 at 12:19 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, Mar 4, 2021 at 9:29 AM Henning Schild > <henning.schild@siemens.com> wrote: > I have given a few comments here and there, so please check the entire > series and address them in _all_ similar locations. As I have noticed, > I have different approach about P2SB code, I have to give the series a > dust and see if you can utilize it. > > I would like to be Cc'ed on the next version. One more thing, is it Apollo Lake based?
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:20:22 +0200 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > On Thu, Mar 4, 2021 at 12:19 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Thu, Mar 4, 2021 at 9:29 AM Henning Schild > > <henning.schild@siemens.com> wrote: > > > I have given a few comments here and there, so please check the > > entire series and address them in _all_ similar locations. As I > > have noticed, I have different approach about P2SB code, I have to > > give the series a dust and see if you can utilize it. > > > > I would like to be Cc'ed on the next version. > > One more thing, is it Apollo Lake based? Not sure i can answer that, or what you even refer to. The whole series is about a range of devices, some even have sub-models with differing CPUs and Lakes regards, Henning
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 :-) -- With Best Regards, Andy Shevchenko
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! -- With Best Regards, Andy Shevchenko
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. -- With Best Regards, Andy Shevchenko
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 -- With Best Regards, Andy Shevchenko
From: Henning Schild <henning.schild@siemens.com> This series adds support for watchdogs and leds of several x86 devices from Siemens. It is structured with a platform driver that mainly does identification of the machines. It might trigger loading of the actual device drivers by attaching devices to the platform bus. The identification is vendor specific, parsing a special binary DMI entry. The implementation of that platform identification is applied on pmc_atom clock quirks in the final patch. It is all structured in a way that we can easily add more devices and more platform drivers later. Internally we have some more code for hardware monitoring, more leds, watchdogs etc. This will follow some day. But the idea here is to share early, and hopefully not fail early. Henning Schild (4): platform/x86: simatic-ipc: add main driver for Siemens devices leds: simatic-ipc-leds: add new driver for Siemens Industial PCs watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs platform/x86: pmc_atom: improve critclk_systems matching for Siemens PCs drivers/leds/Kconfig | 11 + drivers/leds/Makefile | 1 + drivers/leds/simatic-ipc-leds.c | 224 +++++++++++++ drivers/platform/x86/Kconfig | 9 + drivers/platform/x86/Makefile | 3 + drivers/platform/x86/pmc_atom.c | 39 +-- drivers/platform/x86/simatic-ipc.c | 166 ++++++++++ drivers/watchdog/Kconfig | 11 + drivers/watchdog/Makefile | 1 + drivers/watchdog/simatic-ipc-wdt.c | 305 ++++++++++++++++++ .../platform_data/x86/simatic-ipc-base.h | 33 ++ include/linux/platform_data/x86/simatic-ipc.h | 68 ++++ 12 files changed, 853 insertions(+), 18 deletions(-) create mode 100644 drivers/leds/simatic-ipc-leds.c create mode 100644 drivers/platform/x86/simatic-ipc.c create mode 100644 drivers/watchdog/simatic-ipc-wdt.c create mode 100644 include/linux/platform_data/x86/simatic-ipc-base.h create mode 100644 include/linux/platform_data/x86/simatic-ipc.h