* Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue [not found] ` <9B4A1B1917080E46B64F07F2989DADD69AF4DDE7@ORSMSX115.amr.corp.intel.com> @ 2019-03-27 16:02 ` Hans de Goede 2019-03-27 16:47 ` Andy Shevchenko 0 siblings, 1 reply; 24+ messages in thread From: Hans de Goede @ 2019-03-27 16:02 UTC (permalink / raw) To: Fujinaka, Todd, Semyon Verchenko, Kirsher, Jeffrey T, e1000-devel, Stephen Boyd, David Müller, linux-clk, Michael Turquette, Stephen Boyd, Andy Shevchenko, linux-clk Hi, On 3/25/19 9:12 PM, Fujinaka, Todd wrote: > This is going to take a bit of time to see what we need to do. The attachments were stripped but I think just figuring out what they had to change in the Realtek driver will tell us what we need to know. I'm not sure fixing this in the ethernet driver is the best way to go, the board in question is an embedded PC and I've also recently received a bug report about a similar problem with the consumer not requesting the pmc clocks causing a HSIC usb hub to not work. I think that it might be better to restore the CLK_IS_CRITICAL workaround for this, but then only on select boards, based on DMI matching. I've added a bunch of relevant people / lists to the Cc. Andy, Stephen, what is your take on this ? I'm myself starting to believe the DMI based applying of the CLK_IS_CRITICAL workaround is the best solution here. Regards, Hans > > Todd Fujinaka > Software Application Engineer > Datacenter Engineering Group > Intel Corporation > todd.fujinaka@intel.com > > > -----Original Message----- > From: Semyon Verchenko [mailto:semverchenko@factor-ts.ru] > Sent: Monday, March 25, 2019 6:25 AM > To: hdegoede@redhat.com; Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>; e1000-devel@lists.sf.net > Subject: [E1000-devel] igb driver with Intel Atom Bay Trail issue > > Dear Linux/igb maintainers, > > We've encountered problem with igb driver (both one that is distributed with Linux kernel and one which is downloadable from intel.com). After commit 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL") on machine with Intel(R) Atom(TM) CPU E3825 with 4 ethernet cards Intel > I211 only one of cards probed correctly, other ones fail with error -2 (error -5 with driver from intel.com). I've rebuilt kernel with commit 648e921888ad reverted and all 4 interfaces had to probe correctly. > Problem is reproducible at least with kernel 5.0.4 and kernels 4.14.y (actually firstly I've encountered this on 4.14.105 while it worked fine with 4.14.67 so I firstly started to build intermediate versions of kernel and found that problem started to appear on 4.14.77, but since it's reproducible in mainline kernel I think I should inform mainline kernel maintainers about this). Also it is reproducible with igb > 5.3.5.22 from intel.com. I'm attaching kernel logs (journalctl.bad is from stock kernel and journalctl.badfix is from stock kernel with commit 648e921888ad reversed), lspci -vnn output (same about file names) and /proc/cpuinfo. The system is Arch Linux with kernel replaced to stock one (it is not Arch Linux-related problem as it appeared in another system with 4.14 kernel). I think something like Hans de Goede's r8169 patches is required for igb (and maybe other Intel drivers), but I'm not so good with driver developing so it seems too hard for me. > > Regards, > Semyon Verchenko > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue 2019-03-27 16:02 ` [E1000-devel] igb driver with Intel Atom Bay Trail issue Hans de Goede @ 2019-03-27 16:47 ` Andy Shevchenko 2019-03-27 17:31 ` David Müller 2019-03-28 14:48 ` Hans de Goede 0 siblings, 2 replies; 24+ messages in thread From: Andy Shevchenko @ 2019-03-27 16:47 UTC (permalink / raw) To: Hans de Goede Cc: Fujinaka, Todd, Semyon Verchenko, Kirsher, Jeffrey T, e1000-devel, Stephen Boyd, David Müller, linux-clk, Michael Turquette, Stephen Boyd On Wed, Mar 27, 2019 at 05:02:31PM +0100, Hans de Goede wrote: > Hi, > > On 3/25/19 9:12 PM, Fujinaka, Todd wrote: > > This is going to take a bit of time to see what we need to do. The attachments were stripped but I think just figuring out what they had to change in the Realtek driver will tell us what we need to know. > > I'm not sure fixing this in the ethernet driver is the best way to go, > the board in question is an embedded PC and I've also recently received > a bug report about a similar problem with the consumer not requesting > the pmc clocks causing a HSIC usb hub to not work. > > I think that it might be better to restore the CLK_IS_CRITICAL workaround > for this, but then only on select boards, based on DMI matching. > > I've added a bunch of relevant people / lists to the Cc. > > Andy, Stephen, what is your take on this ? I'm afraid I forgot all details about that (semi-)famous issue. Though, looking into your patch against r8169 and taking into account DT practice, it would be not bad to fix a driver, we have by the way devm_clk_get_optional() now, so, it would be not a big deal. > I'm myself starting to believe the DMI based applying of the > CLK_IS_CRITICAL workaround is the best solution here. DMI quirk table tends to grow in mysterious ways. I would prefer in this case logical solution — if platform has an optional clock, then use it. > > Regards, > > Hans > > > > > > Todd Fujinaka > > Software Application Engineer > > Datacenter Engineering Group > > Intel Corporation > > todd.fujinaka@intel.com > > > > > > -----Original Message----- > > From: Semyon Verchenko [mailto:semverchenko@factor-ts.ru] > > Sent: Monday, March 25, 2019 6:25 AM > > To: hdegoede@redhat.com; Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>; e1000-devel@lists.sf.net > > Subject: [E1000-devel] igb driver with Intel Atom Bay Trail issue > > > > Dear Linux/igb maintainers, > > > > We've encountered problem with igb driver (both one that is distributed with Linux kernel and one which is downloadable from intel.com). After commit 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL") on machine with Intel(R) Atom(TM) CPU E3825 with 4 ethernet cards Intel > > I211 only one of cards probed correctly, other ones fail with error -2 (error -5 with driver from intel.com). I've rebuilt kernel with commit 648e921888ad reverted and all 4 interfaces had to probe correctly. > > Problem is reproducible at least with kernel 5.0.4 and kernels 4.14.y (actually firstly I've encountered this on 4.14.105 while it worked fine with 4.14.67 so I firstly started to build intermediate versions of kernel and found that problem started to appear on 4.14.77, but since it's reproducible in mainline kernel I think I should inform mainline kernel maintainers about this). Also it is reproducible with igb > > 5.3.5.22 from intel.com. I'm attaching kernel logs (journalctl.bad is from stock kernel and journalctl.badfix is from stock kernel with commit 648e921888ad reversed), lspci -vnn output (same about file names) and /proc/cpuinfo. The system is Arch Linux with kernel replaced to stock one (it is not Arch Linux-related problem as it appeared in another system with 4.14 kernel). I think something like Hans de Goede's r8169 patches is required for igb (and maybe other Intel drivers), but I'm not so good with driver developing so it seems too hard for me. > > > > Regards, > > Semyon Verchenko > > -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue 2019-03-27 16:47 ` Andy Shevchenko @ 2019-03-27 17:31 ` David Müller 2019-03-27 19:19 ` Andy Shevchenko 2019-03-28 14:48 ` Hans de Goede 1 sibling, 1 reply; 24+ messages in thread From: David Müller @ 2019-03-27 17:31 UTC (permalink / raw) To: Andy Shevchenko, Hans de Goede Cc: Fujinaka, Todd, Kirsher, Jeffrey T, e1000-devel, Stephen Boyd, linux-clk, Michael Turquette, Stephen Boyd Hello Andy Shevchenko wrote: > On Wed, Mar 27, 2019 at 05:02:31PM +0100, Hans de Goede wrote: >> I think that it might be better to restore the CLK_IS_CRITICAL workaround >> for this, but then only on select boards, based on DMI matching. >> >> I've added a bunch of relevant people / lists to the Cc. >> >> Andy, Stephen, what is your take on this ? > > I'm afraid I forgot all details about that (semi-)famous issue. Though, looking > into your patch against r8169 and taking into account DT practice, it would be > not bad to fix a driver, we have by the way devm_clk_get_optional() now, so, it > would be not a big deal. > >> I'm myself starting to believe the DMI based applying of the >> CLK_IS_CRITICAL workaround is the best solution here. > > DMI quirk table tends to grow in mysterious ways. I would prefer in this case > logical solution — if platform has an optional clock, then use it. The pmc_plt_clks may also be used for external hardware purposes without the need for a driver involved. So I'm afraid a fix similar to the r8169 approach will not suit all needs. Please see https://www.spinics.net/lists/linux-clk/msg35800.html for details. I'm in the final phase of testing a DMI based patch on my hardware. If all goes well, I could provide a first version of the patch for review by tomorrow. Dave ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue 2019-03-27 17:31 ` David Müller @ 2019-03-27 19:19 ` Andy Shevchenko 2019-03-28 14:35 ` David Müller 0 siblings, 1 reply; 24+ messages in thread From: Andy Shevchenko @ 2019-03-27 19:19 UTC (permalink / raw) To: David Müller Cc: Hans de Goede, Fujinaka, Todd, Kirsher, Jeffrey T, e1000-devel, Stephen Boyd, linux-clk, Michael Turquette, Stephen Boyd On Wed, Mar 27, 2019 at 06:31:19PM +0100, David Müller wrote: > Andy Shevchenko wrote: > > On Wed, Mar 27, 2019 at 05:02:31PM +0100, Hans de Goede wrote: > > > I think that it might be better to restore the CLK_IS_CRITICAL workaround > > > for this, but then only on select boards, based on DMI matching. > > > > > > I've added a bunch of relevant people / lists to the Cc. > > > > > > Andy, Stephen, what is your take on this ? > > > > I'm afraid I forgot all details about that (semi-)famous issue. Though, looking > > into your patch against r8169 and taking into account DT practice, it would be > > not bad to fix a driver, we have by the way devm_clk_get_optional() now, so, it > > would be not a big deal. > > > > > I'm myself starting to believe the DMI based applying of the > > > CLK_IS_CRITICAL workaround is the best solution here. > > > > DMI quirk table tends to grow in mysterious ways. I would prefer in this case > > logical solution — if platform has an optional clock, then use it. > > The pmc_plt_clks may also be used for external hardware purposes without > the need for a driver involved. So I'm afraid a fix similar to the r8169 > approach will not suit all needs. Please see > https://www.spinics.net/lists/linux-clk/msg35800.html for details. Any driver for device which is using PMC clock should take it into consideration. I don't see any issues with patching devices drivers based on case-by-case approach. Is there any other impediment that prevents us to update the driver in question? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue 2019-03-27 19:19 ` Andy Shevchenko @ 2019-03-28 14:35 ` David Müller 2019-03-28 14:58 ` Andy Shevchenko 0 siblings, 1 reply; 24+ messages in thread From: David Müller @ 2019-03-28 14:35 UTC (permalink / raw) To: Andy Shevchenko Cc: Hans de Goede, Fujinaka, Todd, Kirsher, Jeffrey T, e1000-devel, Stephen Boyd, linux-clk, Michael Turquette, Stephen Boyd Hello Andy Shevchenko wrote: > On Wed, Mar 27, 2019 at 06:31:19PM +0100, David Müller wrote: >> The pmc_plt_clks may also be used for external hardware purposes without >> the need for a driver involved. So I'm afraid a fix similar to the r8169 >> approach will not suit all needs. Please see >> https://www.spinics.net/lists/linux-clk/msg35800.html for details. > > Any driver for device which is using PMC clock should take it into > consideration. I agree that each driver should properly request the clocks and other resources needed. But what if the PMC clock is used by hardware for which no driver is being loaded or needed? Dave ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue 2019-03-28 14:35 ` David Müller @ 2019-03-28 14:58 ` Andy Shevchenko 2019-03-28 15:01 ` Hans de Goede 0 siblings, 1 reply; 24+ messages in thread From: Andy Shevchenko @ 2019-03-28 14:58 UTC (permalink / raw) To: David Müller Cc: Hans de Goede, Fujinaka, Todd, Kirsher, Jeffrey T, e1000-devel, Stephen Boyd, linux-clk, Michael Turquette, Stephen Boyd On Thu, Mar 28, 2019 at 03:35:58PM +0100, David Müller wrote: > Andy Shevchenko wrote: > > On Wed, Mar 27, 2019 at 06:31:19PM +0100, David Müller wrote: > >> The pmc_plt_clks may also be used for external hardware purposes without > >> the need for a driver involved. So I'm afraid a fix similar to the r8169 > >> approach will not suit all needs. Please see > >> https://www.spinics.net/lists/linux-clk/msg35800.html for details. > > > > Any driver for device which is using PMC clock should take it into > > consideration. > > I agree that each driver should properly request the clocks and other > resources needed. > > But what if the PMC clock is used by hardware for which no driver is > being loaded or needed? Perhaps I didn't get a full picture. In the original message you referred to igb _driver_ and devices that are not working properly after resume. The igb driver patching can fix that, right? If the driver is not loaded, then the clock is not in use and can be gated, correct? If there is a connection of this clock to the hardware which is served by firmware, then its firmware's deal, no? Can you elaborate a bit more the case you are talking about? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue 2019-03-28 14:58 ` Andy Shevchenko @ 2019-03-28 15:01 ` Hans de Goede 2019-03-28 15:24 ` Andy Shevchenko 0 siblings, 1 reply; 24+ messages in thread From: Hans de Goede @ 2019-03-28 15:01 UTC (permalink / raw) To: Andy Shevchenko, David Müller Cc: Fujinaka, Todd, Kirsher, Jeffrey T, e1000-devel, Stephen Boyd, linux-clk, Michael Turquette, Stephen Boyd Hi Andy, On 28-03-19 15:58, Andy Shevchenko wrote: > On Thu, Mar 28, 2019 at 03:35:58PM +0100, David Müller wrote: >> Andy Shevchenko wrote: >>> On Wed, Mar 27, 2019 at 06:31:19PM +0100, David Müller wrote: >>>> The pmc_plt_clks may also be used for external hardware purposes without >>>> the need for a driver involved. So I'm afraid a fix similar to the r8169 >>>> approach will not suit all needs. Please see >>>> https://www.spinics.net/lists/linux-clk/msg35800.html for details. >>> >>> Any driver for device which is using PMC clock should take it into >>> consideration. >> >> I agree that each driver should properly request the clocks and other >> resources needed. >> >> But what if the PMC clock is used by hardware for which no driver is >> being loaded or needed? > > Perhaps I didn't get a full picture. > > In the original message you referred to igb _driver_ and devices that are not > working properly after resume. > > The igb driver patching can fix that, right? > > If the driver is not loaded, then the clock is not in use and can be gated, > correct? > > If there is a connection of this clock to the hardware which is served by > firmware, then its firmware's deal, no? > > Can you elaborate a bit more the case you are talking about? In David's case we are talking about a USB hub which needs a pmc-clk (yes really). Also see me other mail I send about this about 5 minutes ago. David's case is the reason why I believe we need a DMI quirk table; and once we have that I think the board with igb ethernet controllers might just as well be handled the same way (I already checked it has usable DMI identifying info). Regards, Hans ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue 2019-03-28 15:01 ` Hans de Goede @ 2019-03-28 15:24 ` Andy Shevchenko 2019-03-28 15:32 ` Hans de Goede 0 siblings, 1 reply; 24+ messages in thread From: Andy Shevchenko @ 2019-03-28 15:24 UTC (permalink / raw) To: Hans de Goede Cc: David Müller, Fujinaka, Todd, Kirsher, Jeffrey T, e1000-devel, Stephen Boyd, linux-clk, Michael Turquette, Stephen Boyd On Thu, Mar 28, 2019 at 04:01:37PM +0100, Hans de Goede wrote: > On 28-03-19 15:58, Andy Shevchenko wrote: > > On Thu, Mar 28, 2019 at 03:35:58PM +0100, David Müller wrote: > > > Andy Shevchenko wrote: > > > > On Wed, Mar 27, 2019 at 06:31:19PM +0100, David Müller wrote: > > > > > The pmc_plt_clks may also be used for external hardware purposes without > > > > > the need for a driver involved. So I'm afraid a fix similar to the r8169 > > > > > approach will not suit all needs. Please see > > > > > https://www.spinics.net/lists/linux-clk/msg35800.html for details. > > > > > > > > Any driver for device which is using PMC clock should take it into > > > > consideration. > > > > > > I agree that each driver should properly request the clocks and other > > > resources needed. > > > > > > But what if the PMC clock is used by hardware for which no driver is > > > being loaded or needed? > > > > Perhaps I didn't get a full picture. > > > > In the original message you referred to igb _driver_ and devices that are not > > working properly after resume. > > > > The igb driver patching can fix that, right? > > > > If the driver is not loaded, then the clock is not in use and can be gated, > > correct? > > > > If there is a connection of this clock to the hardware which is served by > > firmware, then its firmware's deal, no? > > > > Can you elaborate a bit more the case you are talking about? > > In David's case we are talking about a USB hub which needs a pmc-clk > (yes really). OK, this is understandable (what a weird HW design)... > Also see me other mail I send about this about 5 minutes > ago. > > David's case is the reason why I believe we need a DMI quirk table; and > once we have that I think the board with igb ethernet controllers might > just as well be handled the same way (I already checked it has usable > DMI identifying info). But am I right that in the case of igb we will loose power at suspend? Wouldn't be better to patch the driver? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue 2019-03-28 15:24 ` Andy Shevchenko @ 2019-03-28 15:32 ` Hans de Goede 2019-03-28 15:49 ` Andy Shevchenko 0 siblings, 1 reply; 24+ messages in thread From: Hans de Goede @ 2019-03-28 15:32 UTC (permalink / raw) To: Andy Shevchenko Cc: David Müller, Fujinaka, Todd, Kirsher, Jeffrey T, e1000-devel, Stephen Boyd, linux-clk, Michael Turquette, Stephen Boyd Hi, On 28-03-19 16:24, Andy Shevchenko wrote: > On Thu, Mar 28, 2019 at 04:01:37PM +0100, Hans de Goede wrote: >> On 28-03-19 15:58, Andy Shevchenko wrote: >>> On Thu, Mar 28, 2019 at 03:35:58PM +0100, David Müller wrote: >>>> Andy Shevchenko wrote: >>>>> On Wed, Mar 27, 2019 at 06:31:19PM +0100, David Müller wrote: >>>>>> The pmc_plt_clks may also be used for external hardware purposes without >>>>>> the need for a driver involved. So I'm afraid a fix similar to the r8169 >>>>>> approach will not suit all needs. Please see >>>>>> https://www.spinics.net/lists/linux-clk/msg35800.html for details. >>>>> >>>>> Any driver for device which is using PMC clock should take it into >>>>> consideration. >>>> >>>> I agree that each driver should properly request the clocks and other >>>> resources needed. >>>> >>>> But what if the PMC clock is used by hardware for which no driver is >>>> being loaded or needed? >>> >>> Perhaps I didn't get a full picture. >>> >>> In the original message you referred to igb _driver_ and devices that are not >>> working properly after resume. >>> >>> The igb driver patching can fix that, right? >>> >>> If the driver is not loaded, then the clock is not in use and can be gated, >>> correct? >>> >>> If there is a connection of this clock to the hardware which is served by >>> firmware, then its firmware's deal, no? >>> >>> Can you elaborate a bit more the case you are talking about? >> >> In David's case we are talking about a USB hub which needs a pmc-clk >> (yes really). > > OK, this is understandable (what a weird HW design)... > >> Also see me other mail I send about this about 5 minutes >> ago. >> >> David's case is the reason why I believe we need a DMI quirk table; and >> once we have that I think the board with igb ethernet controllers might >> just as well be handled the same way (I already checked it has usable >> DMI identifying info). > > But am I right that in the case of igb we will loose power at suspend? Wouldn't > be better to patch the driver? This is an industrial embedded PC, so it is not running on battery and I doubt it typically spends a lot of time in suspend at all. Regards, Hans ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue 2019-03-28 15:32 ` Hans de Goede @ 2019-03-28 15:49 ` Andy Shevchenko 2019-03-29 4:46 ` Hisashi T Fujinaka 2019-03-29 12:30 ` Hans de Goede 0 siblings, 2 replies; 24+ messages in thread From: Andy Shevchenko @ 2019-03-28 15:49 UTC (permalink / raw) To: Hans de Goede Cc: David Müller, Fujinaka, Todd, Kirsher, Jeffrey T, e1000-devel, Stephen Boyd, linux-clk, Michael Turquette, Stephen Boyd On Thu, Mar 28, 2019 at 04:32:27PM +0100, Hans de Goede wrote: > On 28-03-19 16:24, Andy Shevchenko wrote: > > On Thu, Mar 28, 2019 at 04:01:37PM +0100, Hans de Goede wrote: > > > On 28-03-19 15:58, Andy Shevchenko wrote: > > > > On Thu, Mar 28, 2019 at 03:35:58PM +0100, David Müller wrote: > > > > > Andy Shevchenko wrote: > > > > > > On Wed, Mar 27, 2019 at 06:31:19PM +0100, David Müller wrote: > > > > > > Any driver for device which is using PMC clock should take it into > > > > > > consideration. > > > > > > > > > > I agree that each driver should properly request the clocks and other > > > > > resources needed. > > > > Can you elaborate a bit more the case you are talking about? > > > I think the board with igb ethernet controllers might > > > just as well be handled the same way (I already checked it has usable > > > DMI identifying info). > > > > But am I right that in the case of igb we will loose power at suspend? Wouldn't > > be better to patch the driver? > > This is an industrial embedded PC, so it is not running on battery and > I doubt it typically spends a lot of time in suspend at all. Okay, but still from logical point of view wouldn't be better to fix the driver for such case? At least I see benefits out of this approach: a) less hackish, less quirk code; b) if this happens on non-industrial case it would be better to have in the driver due to power consumption. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue 2019-03-28 15:49 ` Andy Shevchenko @ 2019-03-29 4:46 ` Hisashi T Fujinaka 2019-03-29 12:32 ` Hans de Goede 2019-03-29 12:30 ` Hans de Goede 1 sibling, 1 reply; 24+ messages in thread From: Hisashi T Fujinaka @ 2019-03-29 4:46 UTC (permalink / raw) To: Andy Shevchenko Cc: Hans de Goede, Stephen Boyd, Michael Turquette, Stephen Boyd, e1000-devel, linux-clk On Thu, 28 Mar 2019, Andy Shevchenko wrote: > On Thu, Mar 28, 2019 at 04:32:27PM +0100, Hans de Goede wrote: >> On 28-03-19 16:24, Andy Shevchenko wrote: >>> On Thu, Mar 28, 2019 at 04:01:37PM +0100, Hans de Goede wrote: >>>> On 28-03-19 15:58, Andy Shevchenko wrote: >>>>> On Thu, Mar 28, 2019 at 03:35:58PM +0100, David M?ller wrote: >>>>>> Andy Shevchenko wrote: >>>>>>> On Wed, Mar 27, 2019 at 06:31:19PM +0100, David M?ller wrote: > >>>>>>> Any driver for device which is using PMC clock should take it into >>>>>>> consideration. >>>>>> >>>>>> I agree that each driver should properly request the clocks and other >>>>>> resources needed. > >>>>> Can you elaborate a bit more the case you are talking about? > >>>> I think the board with igb ethernet controllers might >>>> just as well be handled the same way (I already checked it has usable >>>> DMI identifying info). >>> >>> But am I right that in the case of igb we will loose power at suspend? Wouldn't >>> be better to patch the driver? >> >> This is an industrial embedded PC, so it is not running on battery and >> I doubt it typically spends a lot of time in suspend at all. > > Okay, but still from logical point of view wouldn't be better to fix the driver > for such case? At least I see benefits out of this approach: a) less hackish, > less quirk code; b) if this happens on non-industrial case it would be better > to have in the driver due to power consumption. Sorry for replying from home. It's either that or top-posting with Outlook. It sounds to me like Andriy's arguments are counter to what he suggests since we'd have to fix the USB and the Ethernet and that would add more special case code, right? However, I'm just the old igb bug fixer and not a clock guy, and I've spent little time outside of the Ethernet drivers. It really sounded like there were going to be changes to the clock code that would resolve the issue. Were those done? Do I still need to change the igb driver to change the clocks used? -- Todd Fujinaka <todd.fujinaka@intel.com> ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue 2019-03-29 4:46 ` Hisashi T Fujinaka @ 2019-03-29 12:32 ` Hans de Goede 0 siblings, 0 replies; 24+ messages in thread From: Hans de Goede @ 2019-03-29 12:32 UTC (permalink / raw) To: todd.fujinaka, Andy Shevchenko Cc: Stephen Boyd, Michael Turquette, Stephen Boyd, e1000-devel, linux-clk Hi Hisashi, On 3/29/19 5:46 AM, Hisashi T Fujinaka wrote: > On Thu, 28 Mar 2019, Andy Shevchenko wrote: > >> On Thu, Mar 28, 2019 at 04:32:27PM +0100, Hans de Goede wrote: >>> On 28-03-19 16:24, Andy Shevchenko wrote: >>>> On Thu, Mar 28, 2019 at 04:01:37PM +0100, Hans de Goede wrote: >>>>> On 28-03-19 15:58, Andy Shevchenko wrote: >>>>>> On Thu, Mar 28, 2019 at 03:35:58PM +0100, David M?ller wrote: >>>>>>> Andy Shevchenko wrote: >>>>>>>> On Wed, Mar 27, 2019 at 06:31:19PM +0100, David M?ller wrote: >> >>>>>>>> Any driver for device which is using PMC clock should take it into >>>>>>>> consideration. >>>>>>> >>>>>>> I agree that each driver should properly request the clocks and other >>>>>>> resources needed. >> >>>>>> Can you elaborate a bit more the case you are talking about? >> >>>>> I think the board with igb ethernet controllers might >>>>> just as well be handled the same way (I already checked it has usable >>>>> DMI identifying info). >>>> >>>> But am I right that in the case of igb we will loose power at suspend? Wouldn't >>>> be better to patch the driver? >>> >>> This is an industrial embedded PC, so it is not running on battery and >>> I doubt it typically spends a lot of time in suspend at all. >> >> Okay, but still from logical point of view wouldn't be better to fix the driver >> for such case? At least I see benefits out of this approach: a) less hackish, >> less quirk code; b) if this happens on non-industrial case it would be better >> to have in the driver due to power consumption. > > Sorry for replying from home. It's either that or top-posting with > Outlook. > > It sounds to me like Andriy's arguments are counter to what he suggests > since we'd have to fix the USB and the Ethernet and that would add more > special case code, right? However, I'm just the old igb bug fixer and > not a clock guy, and I've spent little time outside of the Ethernet > drivers. > > It really sounded like there were going to be changes to the clock code > that would resolve the issue. Were those done? Do I still need to change > the igb driver to change the clocks used? Andy and I are still discussing this, we will get back to you when we've a conclusion on how best to fix this. Regards, Hans ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue 2019-03-28 15:49 ` Andy Shevchenko 2019-03-29 4:46 ` Hisashi T Fujinaka @ 2019-03-29 12:30 ` Hans de Goede 2019-03-29 13:59 ` Семен Верченко 1 sibling, 1 reply; 24+ messages in thread From: Hans de Goede @ 2019-03-29 12:30 UTC (permalink / raw) To: Andy Shevchenko Cc: David Müller, Fujinaka, Todd, Kirsher, Jeffrey T, e1000-devel, Stephen Boyd, linux-clk, Michael Turquette, Stephen Boyd, Semyon Verchenko Hi, On 3/28/19 4:49 PM, Andy Shevchenko wrote: > On Thu, Mar 28, 2019 at 04:32:27PM +0100, Hans de Goede wrote: >> On 28-03-19 16:24, Andy Shevchenko wrote: >>> On Thu, Mar 28, 2019 at 04:01:37PM +0100, Hans de Goede wrote: >>>> On 28-03-19 15:58, Andy Shevchenko wrote: >>>>> On Thu, Mar 28, 2019 at 03:35:58PM +0100, David Müller wrote: >>>>>> Andy Shevchenko wrote: >>>>>>> On Wed, Mar 27, 2019 at 06:31:19PM +0100, David Müller wrote: > >>>>>>> Any driver for device which is using PMC clock should take it into >>>>>>> consideration. >>>>>> >>>>>> I agree that each driver should properly request the clocks and other >>>>>> resources needed. > >>>>> Can you elaborate a bit more the case you are talking about? > >>>> I think the board with igb ethernet controllers might >>>> just as well be handled the same way (I already checked it has usable >>>> DMI identifying info). >>> >>> But am I right that in the case of igb we will loose power at suspend? Wouldn't >>> be better to patch the driver? >> >> This is an industrial embedded PC, so it is not running on battery and >> I doubt it typically spends a lot of time in suspend at all. > > Okay, but still from logical point of view wouldn't be better to fix the driver > for such case? At least I see benefits out of this approach: a) less hackish, > less quirk code; b) if this happens on non-industrial case it would be better > to have in the driver due to power consumption. Maybe, I guess we first need to figure out which platforms clock(s) is (are) being used, if there is more then one; or it is a different one then in the realtek ethernet case it might be better to go with the dmi quirk option. Semyon Verchenko can you (as root) run the following command on a kernel where the ethernet does work: grep . /sys/kernel/debug/clk/pmc_plt_clk_?/flags And then email us the output please? Regards, Hans ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue 2019-03-29 12:30 ` Hans de Goede @ 2019-03-29 13:59 ` Семен Верченко 2019-03-29 15:53 ` Hans de Goede 0 siblings, 1 reply; 24+ messages in thread From: Семен Верченко @ 2019-03-29 13:59 UTC (permalink / raw) To: Hans de Goede, Andy Shevchenko Cc: David Müller, Fujinaka, Todd, Kirsher, Jeffrey T, e1000-devel, Stephen Boyd, linux-clk, Michael Turquette, Stephen Boyd On 29.03.2019 15:30, Hans de Goede wrote: > Hi, > > On 3/28/19 4:49 PM, Andy Shevchenko wrote: >> On Thu, Mar 28, 2019 at 04:32:27PM +0100, Hans de Goede wrote: >>> On 28-03-19 16:24, Andy Shevchenko wrote: >>>> On Thu, Mar 28, 2019 at 04:01:37PM +0100, Hans de Goede wrote: >>>>> On 28-03-19 15:58, Andy Shevchenko wrote: >>>>>> On Thu, Mar 28, 2019 at 03:35:58PM +0100, David Müller wrote: >>>>>>> Andy Shevchenko wrote: >>>>>>>> On Wed, Mar 27, 2019 at 06:31:19PM +0100, David Müller wrote: >> >>>>>>>> Any driver for device which is using PMC clock should take it into >>>>>>>> consideration. >>>>>>> >>>>>>> I agree that each driver should properly request the clocks and >>>>>>> other >>>>>>> resources needed. >> >>>>>> Can you elaborate a bit more the case you are talking about? >> >>>>> I think the board with igb ethernet controllers might >>>>> just as well be handled the same way (I already checked it has usable >>>>> DMI identifying info). >>>> >>>> But am I right that in the case of igb we will loose power at >>>> suspend? Wouldn't >>>> be better to patch the driver? >>> >>> This is an industrial embedded PC, so it is not running on battery and >>> I doubt it typically spends a lot of time in suspend at all. >> >> Okay, but still from logical point of view wouldn't be better to fix >> the driver >> for such case? At least I see benefits out of this approach: a) less >> hackish, >> less quirk code; b) if this happens on non-industrial case it would >> be better >> to have in the driver due to power consumption. > > Maybe, I guess we first need to figure out which platforms clock(s) is > (are) > being used, if there is more then one; or it is a different one then > in the > realtek ethernet case it might be better to go with the dmi quirk option. > > Semyon Verchenko can you (as root) run the following command on a kernel > where the ethernet does work: > > grep . /sys/kernel/debug/clk/pmc_plt_clk_?/flags > > And then email us the output please? > > Regards, > > Hans > I don't have flags files in /sys/kernel/debug/clk/pmc_plt_clk_?; did you mean clk_flags? [root@archatom ~]# grep . /sys/kernel/debug/clk/pmc_plt_clk_?/flags grep: /sys/kernel/debug/clk/pmc_plt_clk_?/flags: No such file or directory [root@archatom ~]# grep . /sys/kernel/debug/clk/pmc_plt_clk_?/clk_flags /sys/kernel/debug/clk/pmc_plt_clk_0/clk_flags:CLK_IS_CRITICAL /sys/kernel/debug/clk/pmc_plt_clk_1/clk_flags:CLK_IS_CRITICAL /sys/kernel/debug/clk/pmc_plt_clk_2/clk_flags:CLK_IS_CRITICAL /sys/kernel/debug/clk/pmc_plt_clk_3/clk_flags:CLK_IS_CRITICAL [root@archatom ~]# ls /sys/kernel/debug/clk clk_dump clk_orphan_summary pll pmc_plt_clk_1 pmc_plt_clk_3 pmc_plt_clk_5 clk_orphan_dump clk_summary pmc_plt_clk_0 pmc_plt_clk_2 pmc_plt_clk_4 xtal Kernel is 5.0.4 with commit 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL") reversed, is it enough or I need to install older kernel? ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue 2019-03-29 13:59 ` Семен Верченко @ 2019-03-29 15:53 ` Hans de Goede 2019-04-04 14:43 ` Hans de Goede 0 siblings, 1 reply; 24+ messages in thread From: Hans de Goede @ 2019-03-29 15:53 UTC (permalink / raw) To: Семен Верченко, Andy Shevchenko Cc: David Müller, Fujinaka, Todd, Kirsher, Jeffrey T, e1000-devel, Stephen Boyd, linux-clk, Michael Turquette, Stephen Boyd Hi, On 3/29/19 2:59 PM, Семен Верченко wrote: > > On 29.03.2019 15:30, Hans de Goede wrote: >> Hi, >> >> On 3/28/19 4:49 PM, Andy Shevchenko wrote: >>> On Thu, Mar 28, 2019 at 04:32:27PM +0100, Hans de Goede wrote: >>>> On 28-03-19 16:24, Andy Shevchenko wrote: >>>>> On Thu, Mar 28, 2019 at 04:01:37PM +0100, Hans de Goede wrote: >>>>>> On 28-03-19 15:58, Andy Shevchenko wrote: >>>>>>> On Thu, Mar 28, 2019 at 03:35:58PM +0100, David Müller wrote: >>>>>>>> Andy Shevchenko wrote: >>>>>>>>> On Wed, Mar 27, 2019 at 06:31:19PM +0100, David Müller wrote: >>> >>>>>>>>> Any driver for device which is using PMC clock should take it into >>>>>>>>> consideration. >>>>>>>> >>>>>>>> I agree that each driver should properly request the clocks and other >>>>>>>> resources needed. >>> >>>>>>> Can you elaborate a bit more the case you are talking about? >>> >>>>>> I think the board with igb ethernet controllers might >>>>>> just as well be handled the same way (I already checked it has usable >>>>>> DMI identifying info). >>>>> >>>>> But am I right that in the case of igb we will loose power at suspend? Wouldn't >>>>> be better to patch the driver? >>>> >>>> This is an industrial embedded PC, so it is not running on battery and >>>> I doubt it typically spends a lot of time in suspend at all. >>> >>> Okay, but still from logical point of view wouldn't be better to fix the driver >>> for such case? At least I see benefits out of this approach: a) less hackish, >>> less quirk code; b) if this happens on non-industrial case it would be better >>> to have in the driver due to power consumption. >> >> Maybe, I guess we first need to figure out which platforms clock(s) is (are) >> being used, if there is more then one; or it is a different one then in the >> realtek ethernet case it might be better to go with the dmi quirk option. >> >> Semyon Verchenko can you (as root) run the following command on a kernel >> where the ethernet does work: >> >> grep . /sys/kernel/debug/clk/pmc_plt_clk_?/flags >> >> And then email us the output please? >> >> Regards, >> >> Hans >> > I don't have flags files in /sys/kernel/debug/clk/pmc_plt_clk_?; did you mean clk_flags? > > [root@archatom ~]# grep . /sys/kernel/debug/clk/pmc_plt_clk_?/flags > grep: /sys/kernel/debug/clk/pmc_plt_clk_?/flags: No such file or directory > [root@archatom ~]# grep . /sys/kernel/debug/clk/pmc_plt_clk_?/clk_flags > /sys/kernel/debug/clk/pmc_plt_clk_0/clk_flags:CLK_IS_CRITICAL > /sys/kernel/debug/clk/pmc_plt_clk_1/clk_flags:CLK_IS_CRITICAL > /sys/kernel/debug/clk/pmc_plt_clk_2/clk_flags:CLK_IS_CRITICAL > /sys/kernel/debug/clk/pmc_plt_clk_3/clk_flags:CLK_IS_CRITICAL > [root@archatom ~]# ls /sys/kernel/debug/clk > clk_dump clk_orphan_summary pll pmc_plt_clk_1 pmc_plt_clk_3 pmc_plt_clk_5 > clk_orphan_dump clk_summary pmc_plt_clk_0 pmc_plt_clk_2 pmc_plt_clk_4 xtal Hmm, so 4 ethernet cards and 4 enabled / marked as critical clocks. Supporting this through get_clk is going to require a DMI table in the igb driver combined with checking which PCI "slot" the card is to get the correct clock for each ethernet controller. I believe tht just restoring the old behavior to mark all clocks enabled on boot as critical, but then limited to this system based on a dmi match, is the best solution here. Andy? Regards, Hans ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue 2019-03-29 15:53 ` Hans de Goede @ 2019-04-04 14:43 ` Hans de Goede [not found] ` <20190408172111.GX9224@smile.fi.intel.com> 0 siblings, 1 reply; 24+ messages in thread From: Hans de Goede @ 2019-04-04 14:43 UTC (permalink / raw) To: Семен Верченко, Andy Shevchenko Cc: David Müller, Fujinaka, Todd, Kirsher, Jeffrey T, e1000-devel, Stephen Boyd, linux-clk, Michael Turquette, Stephen Boyd Hi, On 29-03-19 16:53, Hans de Goede wrote: > Hi, > > On 3/29/19 2:59 PM, Семен Верченко wrote: >> >> On 29.03.2019 15:30, Hans de Goede wrote: >>> Hi, >>> >>> On 3/28/19 4:49 PM, Andy Shevchenko wrote: >>>> On Thu, Mar 28, 2019 at 04:32:27PM +0100, Hans de Goede wrote: >>>>> On 28-03-19 16:24, Andy Shevchenko wrote: >>>>>> On Thu, Mar 28, 2019 at 04:01:37PM +0100, Hans de Goede wrote: >>>>>>> On 28-03-19 15:58, Andy Shevchenko wrote: >>>>>>>> On Thu, Mar 28, 2019 at 03:35:58PM +0100, David Müller wrote: >>>>>>>>> Andy Shevchenko wrote: >>>>>>>>>> On Wed, Mar 27, 2019 at 06:31:19PM +0100, David Müller wrote: >>>> >>>>>>>>>> Any driver for device which is using PMC clock should take it into >>>>>>>>>> consideration. >>>>>>>>> >>>>>>>>> I agree that each driver should properly request the clocks and other >>>>>>>>> resources needed. >>>> >>>>>>>> Can you elaborate a bit more the case you are talking about? >>>> >>>>>>> I think the board with igb ethernet controllers might >>>>>>> just as well be handled the same way (I already checked it has usable >>>>>>> DMI identifying info). >>>>>> >>>>>> But am I right that in the case of igb we will loose power at suspend? Wouldn't >>>>>> be better to patch the driver? >>>>> >>>>> This is an industrial embedded PC, so it is not running on battery and >>>>> I doubt it typically spends a lot of time in suspend at all. >>>> >>>> Okay, but still from logical point of view wouldn't be better to fix the driver >>>> for such case? At least I see benefits out of this approach: a) less hackish, >>>> less quirk code; b) if this happens on non-industrial case it would be better >>>> to have in the driver due to power consumption. >>> >>> Maybe, I guess we first need to figure out which platforms clock(s) is (are) >>> being used, if there is more then one; or it is a different one then in the >>> realtek ethernet case it might be better to go with the dmi quirk option. >>> >>> Semyon Verchenko can you (as root) run the following command on a kernel >>> where the ethernet does work: >>> >>> grep . /sys/kernel/debug/clk/pmc_plt_clk_?/flags >>> >>> And then email us the output please? >>> >>> Regards, >>> >>> Hans >>> >> I don't have flags files in /sys/kernel/debug/clk/pmc_plt_clk_?; did you mean clk_flags? >> >> [root@archatom ~]# grep . /sys/kernel/debug/clk/pmc_plt_clk_?/flags >> grep: /sys/kernel/debug/clk/pmc_plt_clk_?/flags: No such file or directory >> [root@archatom ~]# grep . /sys/kernel/debug/clk/pmc_plt_clk_?/clk_flags >> /sys/kernel/debug/clk/pmc_plt_clk_0/clk_flags:CLK_IS_CRITICAL >> /sys/kernel/debug/clk/pmc_plt_clk_1/clk_flags:CLK_IS_CRITICAL >> /sys/kernel/debug/clk/pmc_plt_clk_2/clk_flags:CLK_IS_CRITICAL >> /sys/kernel/debug/clk/pmc_plt_clk_3/clk_flags:CLK_IS_CRITICAL >> [root@archatom ~]# ls /sys/kernel/debug/clk >> clk_dump clk_orphan_summary pll pmc_plt_clk_1 pmc_plt_clk_3 pmc_plt_clk_5 >> clk_orphan_dump clk_summary pmc_plt_clk_0 pmc_plt_clk_2 pmc_plt_clk_4 xtal > > Hmm, so 4 ethernet cards and 4 enabled / marked as critical clocks. > > Supporting this through get_clk is going to require a DMI table in the igb driver > combined with checking which PCI "slot" the card is to get the correct clock > for each ethernet controller. > > I believe tht just restoring the old behavior to mark all clocks enabled > on boot as critical, but then limited to this system based on a dmi match, > is the best solution here. > > Andy? Andy? Now that we've the patch ready for the other system which needs to have the CLK_IS_CRITICAL workaround and enables this based on DMI info, I believe the best fix for this system is to simply add it to that DMI table? Regards, Hans ^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <20190408172111.GX9224@smile.fi.intel.com>]
* Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue [not found] ` <20190408172111.GX9224@smile.fi.intel.com> @ 2019-04-08 18:43 ` Hans de Goede 2019-04-09 15:31 ` Andy Shevchenko 0 siblings, 1 reply; 24+ messages in thread From: Hans de Goede @ 2019-04-08 18:43 UTC (permalink / raw) To: Andy Shevchenko Cc: Семен Верченко, David Müller, Fujinaka, Todd, Kirsher, Jeffrey T, e1000-devel, Stephen Boyd, linux-clk, Michael Turquette, Stephen Boyd Hi, On 08-04-19 19:21, Andy Shevchenko wrote: > On Thu, Apr 04, 2019 at 04:43:03PM +0200, Hans de Goede wrote: >> On 29-03-19 16:53, Hans de Goede wrote: >>> On 3/29/19 2:59 PM, Семен Верченко wrote: > >>> Hmm, so 4 ethernet cards and 4 enabled / marked as critical clocks. >>> >>> Supporting this through get_clk is going to require a DMI table in the igb driver >>> combined with checking which PCI "slot" the card is to get the correct clock >>> for each ethernet controller. >>> >>> I believe tht just restoring the old behavior to mark all clocks enabled >>> on boot as critical, but then limited to this system based on a dmi match, >>> is the best solution here. >>> >>> Andy? >> >> Andy? Now that we've the patch ready for the other system which needs to >> have the CLK_IS_CRITICAL workaround and enables this based on DMI info, >> I believe the best fix for this system is to simply add it to that DMI >> table? > > I reviewed v4, supposed to go via CLK tree. Right, but that patch adds the quirk for the system with the USB hub, do you agree, that given that each ethernet controller seems to be using its own clock, it is best to use a DMI quirk for this case too? If you agree then someone needs to prepare a follow-up patch on top of v4 which adds the DMI info for this board to the table. Regards, Hans ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue 2019-04-08 18:43 ` Hans de Goede @ 2019-04-09 15:31 ` Andy Shevchenko 2019-04-18 13:09 ` Hans de Goede 0 siblings, 1 reply; 24+ messages in thread From: Andy Shevchenko @ 2019-04-09 15:31 UTC (permalink / raw) To: Hans de Goede Cc: Семен Верченко, David Müller, Fujinaka, Todd, Kirsher, Jeffrey T, e1000-devel, Stephen Boyd, linux-clk, Michael Turquette, Stephen Boyd On Mon, Apr 08, 2019 at 08:43:10PM +0200, Hans de Goede wrote: > On 08-04-19 19:21, Andy Shevchenko wrote: > > On Thu, Apr 04, 2019 at 04:43:03PM +0200, Hans de Goede wrote: > > > On 29-03-19 16:53, Hans de Goede wrote: > > > > On 3/29/19 2:59 PM, Семен Верченко wrote: > > > > > > Hmm, so 4 ethernet cards and 4 enabled / marked as critical clocks. > > > > > > > > Supporting this through get_clk is going to require a DMI table in the igb driver > > > > combined with checking which PCI "slot" the card is to get the correct clock > > > > for each ethernet controller. > > > > > > > > I believe tht just restoring the old behavior to mark all clocks enabled > > > > on boot as critical, but then limited to this system based on a dmi match, > > > > is the best solution here. > > > > > > > > Andy? > > > > > > Andy? Now that we've the patch ready for the other system which needs to > > > have the CLK_IS_CRITICAL workaround and enables this based on DMI info, > > > I believe the best fix for this system is to simply add it to that DMI > > > table? > > > > I reviewed v4, supposed to go via CLK tree. > > Right, but that patch adds the quirk for the system with the USB hub, > do you agree, that given that each ethernet controller seems to be > using its own clock, it is best to use a DMI quirk for this case too? > > If you agree then someone needs to prepare a follow-up patch on top of > v4 which adds the DMI info for this board to the table. I hope we may find a better solution in the future, but for now as a quick fix the proposed can be done. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue 2019-04-09 15:31 ` Andy Shevchenko @ 2019-04-18 13:09 ` Hans de Goede 2019-04-18 13:26 ` Semyon Verchenko 0 siblings, 1 reply; 24+ messages in thread From: Hans de Goede @ 2019-04-18 13:09 UTC (permalink / raw) To: Andy Shevchenko Cc: Семен Верченко, David Müller, Fujinaka, Todd, Kirsher, Jeffrey T, e1000-devel, Stephen Boyd, linux-clk, Michael Turquette, Stephen Boyd Hi, On 09-04-19 17:31, Andy Shevchenko wrote: > On Mon, Apr 08, 2019 at 08:43:10PM +0200, Hans de Goede wrote: >> On 08-04-19 19:21, Andy Shevchenko wrote: >>> On Thu, Apr 04, 2019 at 04:43:03PM +0200, Hans de Goede wrote: >>>> On 29-03-19 16:53, Hans de Goede wrote: >>>>> On 3/29/19 2:59 PM, Семен Верченко wrote: >>> >>>>> Hmm, so 4 ethernet cards and 4 enabled / marked as critical clocks. >>>>> >>>>> Supporting this through get_clk is going to require a DMI table in the igb driver >>>>> combined with checking which PCI "slot" the card is to get the correct clock >>>>> for each ethernet controller. >>>>> >>>>> I believe tht just restoring the old behavior to mark all clocks enabled >>>>> on boot as critical, but then limited to this system based on a dmi match, >>>>> is the best solution here. >>>>> >>>>> Andy? >>>> >>>> Andy? Now that we've the patch ready for the other system which needs to >>>> have the CLK_IS_CRITICAL workaround and enables this based on DMI info, >>>> I believe the best fix for this system is to simply add it to that DMI >>>> table? >>> >>> I reviewed v4, supposed to go via CLK tree. >> >> Right, but that patch adds the quirk for the system with the USB hub, >> do you agree, that given that each ethernet controller seems to be >> using its own clock, it is best to use a DMI quirk for this case too? >> >> If you agree then someone needs to prepare a follow-up patch on top of >> v4 which adds the DMI info for this board to the table. > > I hope we may find a better solution in the future, but for now as a quick fix > the proposed can be done. Ok. Семен Верченко, this means that we are going to need DMI info from the board in question. I thought we already had that, but I now see that you original report did not have that a Please run as root: dmidecode &> dmidecode.log And then reply to this email with the generated dmidecode.log file attached. Once I have that file I can prepare a patch fixing this. Regards, Hans ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue 2019-04-18 13:09 ` Hans de Goede @ 2019-04-18 13:26 ` Semyon Verchenko 2019-04-18 15:12 ` Hans de Goede 0 siblings, 1 reply; 24+ messages in thread From: Semyon Verchenko @ 2019-04-18 13:26 UTC (permalink / raw) To: Hans de Goede, Andy Shevchenko Cc: David Müller, Fujinaka, Todd, Kirsher, Jeffrey T, e1000-devel, Stephen Boyd, linux-clk, Michael Turquette, Stephen Boyd [-- Attachment #1: Type: text/plain, Size: 2171 bytes --] On 18.04.2019 16:09, Hans de Goede wrote: > Hi, > > On 09-04-19 17:31, Andy Shevchenko wrote: >> On Mon, Apr 08, 2019 at 08:43:10PM +0200, Hans de Goede wrote: >>> On 08-04-19 19:21, Andy Shevchenko wrote: >>>> On Thu, Apr 04, 2019 at 04:43:03PM +0200, Hans de Goede wrote: >>>>> On 29-03-19 16:53, Hans de Goede wrote: >>>>>> On 3/29/19 2:59 PM, Семен Верченко wrote: >>>> >>>>>> Hmm, so 4 ethernet cards and 4 enabled / marked as critical clocks. >>>>>> >>>>>> Supporting this through get_clk is going to require a DMI table >>>>>> in the igb driver >>>>>> combined with checking which PCI "slot" the card is to get the >>>>>> correct clock >>>>>> for each ethernet controller. >>>>>> >>>>>> I believe tht just restoring the old behavior to mark all clocks >>>>>> enabled >>>>>> on boot as critical, but then limited to this system based on a >>>>>> dmi match, >>>>>> is the best solution here. >>>>>> >>>>>> Andy? >>>>> >>>>> Andy? Now that we've the patch ready for the other system which >>>>> needs to >>>>> have the CLK_IS_CRITICAL workaround and enables this based on DMI >>>>> info, >>>>> I believe the best fix for this system is to simply add it to that >>>>> DMI >>>>> table? >>>> >>>> I reviewed v4, supposed to go via CLK tree. >>> >>> Right, but that patch adds the quirk for the system with the USB hub, >>> do you agree, that given that each ethernet controller seems to be >>> using its own clock, it is best to use a DMI quirk for this case too? >>> >>> If you agree then someone needs to prepare a follow-up patch on top of >>> v4 which adds the DMI info for this board to the table. >> >> I hope we may find a better solution in the future, but for now as a >> quick fix >> the proposed can be done. > > Ok. > > Семен Верченко, this means that we are going to need DMI info from > the board in question. I thought we already had that, but I now see that > you original report did not have that a > > Please run as root: > > dmidecode &> dmidecode.log > > And then reply to this email with the generated dmidecode.log file > attached. Once I have that file I can prepare a patch fixing this. > > Regards, > > Hans > [-- Attachment #2: dmidecode.log --] [-- Type: text/x-log, Size: 15787 bytes --] # dmidecode 3.2 Getting SMBIOS data from sysfs. SMBIOS 2.7 present. 51 structures occupying 2122 bytes. Table at 0x000E6F40. Handle 0x0000, DMI type 0, 24 bytes BIOS Information Vendor: INSYDE Corp. Version: 3I380D A3 Release Date: 11/09/2015 Address: 0xF0000 Runtime Size: 64 kB ROM Size: 3072 kB Characteristics: PCI is supported BIOS is upgradeable BIOS shadowing is allowed Boot from CD is supported Selectable boot is supported EDD is supported 8042 keyboard services are supported (int 9h) Serial services are supported (int 14h) Printer services are supported (int 17h) CGA/mono video services are supported (int 10h) ACPI is supported USB legacy is supported ATAPI Zip drive boot is supported BIOS boot specification is supported Function key-initiated network boot is supported Targeted content distribution is supported BIOS Revision: 24.0 Firmware Revision: 0.0 Handle 0x0001, DMI type 1, 27 bytes System Information Manufacturer: Lex BayTrail Product Name: 3I380D Version: Serial Number: BT-I C2II1 UUID: 00000001-0000-4000-0000-4c028910f18b Wake-up Type: Power Switch SKU Number: Family: Handle 0x0002, DMI type 2, 17 bytes Base Board Information Manufacturer: Lex Product Name: 3I380D Version: Serial Number: Asset Tag: Features: Board is a hosting board Board is replaceable Location In Chassis: Chassis Handle: 0x0003 Type: Motherboard Contained Object Handles: 0 Handle 0x0003, DMI type 3, 24 bytes Chassis Information Manufacturer: Lex Type: Notebook Lock: Not Present Version: Serial Number: Asset Tag: Boot-up State: Safe Power Supply State: Safe Thermal State: Safe Security Status: None OEM Information: 0x00000000 Height: Unspecified Number Of Power Cords: 1 Contained Elements: 0 SKU Number: Not Specified Handle 0x0004, DMI type 4, 42 bytes Processor Information Socket Designation: CPU 1 Type: Central Processor Family: Atom Manufacturer: Intel(R) Corporation ID: 79 06 03 00 FF FB EB BF Signature: Type 0, Family 6, Model 55, Stepping 9 Flags: FPU (Floating-point unit on-chip) VME (Virtual mode extension) DE (Debugging extension) PSE (Page size extension) TSC (Time stamp counter) MSR (Model specific registers) PAE (Physical address extension) MCE (Machine check exception) CX8 (CMPXCHG8 instruction supported) APIC (On-chip APIC hardware supported) SEP (Fast system call) MTRR (Memory type range registers) PGE (Page global enable) MCA (Machine check architecture) CMOV (Conditional move instruction supported) PAT (Page attribute table) PSE-36 (36-bit page size extension) CLFSH (CLFLUSH instruction supported) DS (Debug store) ACPI (ACPI supported) MMX (MMX technology supported) FXSR (FXSAVE and FXSTOR instructions supported) SSE (Streaming SIMD extensions) SSE2 (Streaming SIMD extensions 2) SS (Self-snoop) HTT (Multi-threading) TM (Thermal monitor supported) PBE (Pending break enabled) Version: Intel(R) Atom(TM) CPU E3825 @ 1.33GHz Voltage: 0.8 V External Clock: 133 MHz Max Speed: 1330 MHz Current Speed: 1340 MHz Status: Populated, Enabled Upgrade: None L1 Cache Handle: 0x0008 L2 Cache Handle: 0x0009 L3 Cache Handle: Not Provided Serial Number: Not Specified Asset Tag: Not Specified Part Number: Not Specified Core Count: 2 Core Enabled: 2 Thread Count: 1 Characteristics: 64-bit capable Multi-Core Execute Protection Enhanced Virtualization Power/Performance Control Handle 0x0005, DMI type 6, 12 bytes Memory Module Information Socket Designation: DIMM0 Bank Connections: 0 0 Current Speed: Unknown Type: None Installed Size: 4096 MB (Single-bank Connection) Enabled Size: 4096 MB (Single-bank Connection) Error Status: OK Handle 0x0006, DMI type 6, 12 bytes Memory Module Information Socket Designation: DIMM1 Bank Connections: 0 0 Current Speed: Unknown Type: None Installed Size: Not Installed Enabled Size: Not Installed Error Status: OK Handle 0x0007, DMI type 7, 19 bytes Cache Information Socket Designation: L1 Cache Configuration: Enabled, Not Socketed, Level 1 Operational Mode: Write Back Location: Internal Installed Size: 48 kB Maximum Size: 48 kB Supported SRAM Types: Synchronous Installed SRAM Type: Synchronous Speed: Unknown Error Correction Type: Single-bit ECC System Type: Data Associativity: Fully Associative Handle 0x0008, DMI type 7, 19 bytes Cache Information Socket Designation: L1 Cache Configuration: Enabled, Not Socketed, Level 1 Operational Mode: Write Back Location: Internal Installed Size: 64 kB Maximum Size: 64 kB Supported SRAM Types: Synchronous Installed SRAM Type: Synchronous Speed: Unknown Error Correction Type: Single-bit ECC System Type: Instruction Associativity: 8-way Set-associative Handle 0x0009, DMI type 7, 19 bytes Cache Information Socket Designation: L2 Cache Configuration: Enabled, Not Socketed, Level 2 Operational Mode: Write Back Location: Internal Installed Size: 1024 kB Maximum Size: 1024 kB Supported SRAM Types: Synchronous Installed SRAM Type: Synchronous Speed: Unknown Error Correction Type: Single-bit ECC System Type: Unified Associativity: 8-way Set-associative Handle 0x000A, DMI type 8, 9 bytes Port Connector Information Internal Reference Designator: J1A2 Internal Connector Type: None External Reference Designator: USB2.0 External Connector Type: Access Bus (USB) Port Type: USB Handle 0x000B, DMI type 8, 9 bytes Port Connector Information Internal Reference Designator: J1A2 Internal Connector Type: None External Reference Designator: USB2.0 External Connector Type: Access Bus (USB) Port Type: USB Handle 0x000C, DMI type 8, 9 bytes Port Connector Information Internal Reference Designator: J2A2 Internal Connector Type: None External Reference Designator: USB3.0 Port0 External Connector Type: Access Bus (USB) Port Type: USB Handle 0x000D, DMI type 8, 9 bytes Port Connector Information Internal Reference Designator: J9D1 Internal Connector Type: None External Reference Designator: UART to Micro USB External Connector Type: Access Bus (USB) Port Type: USB Handle 0x000E, DMI type 8, 9 bytes Port Connector Information Internal Reference Designator: CON6A1 Internal Connector Type: None External Reference Designator: SD Card External Connector Type: Other Port Type: Other Handle 0x000F, DMI type 8, 9 bytes Port Connector Information Internal Reference Designator: J9B1 Internal Connector Type: None External Reference Designator: Keyboard External Connector Type: PS/2 Port Type: Keyboard Port Handle 0x0010, DMI type 8, 9 bytes Port Connector Information Internal Reference Designator: J9B1 Internal Connector Type: None External Reference Designator: Mouse External Connector Type: PS/2 Port Type: Mouse Port Handle 0x0011, DMI type 8, 9 bytes Port Connector Information Internal Reference Designator: J5A1 Internal Connector Type: None External Reference Designator: Microphone External Connector Type: Mini Jack (headphones) Port Type: Audio Port Handle 0x0012, DMI type 8, 9 bytes Port Connector Information Internal Reference Designator: J5A1 Internal Connector Type: None External Reference Designator: Line In External Connector Type: Mini Jack (headphones) Port Type: Audio Port Handle 0x0013, DMI type 8, 9 bytes Port Connector Information Internal Reference Designator: J5A1 Internal Connector Type: None External Reference Designator: Line Out External Connector Type: Mini Jack (headphones) Port Type: Audio Port Handle 0x0014, DMI type 8, 9 bytes Port Connector Information Internal Reference Designator: J7A1 Internal Connector Type: None External Reference Designator: Audio Jack External Connector Type: Mini Jack (headphones) Port Type: Audio Port Handle 0x0015, DMI type 8, 9 bytes Port Connector Information Internal Reference Designator: J2A2 Internal Connector Type: None External Reference Designator: Network Rj45 Jack External Connector Type: RJ-45 Port Type: Network Port Handle 0x0016, DMI type 8, 9 bytes Port Connector Information Internal Reference Designator: J4J1 Internal Connector Type: None External Reference Designator: SATA External Connector Type: SAS/SATA Plug Receptacle Port Type: Other Handle 0x0017, DMI type 8, 9 bytes Port Connector Information Internal Reference Designator: J4E2 Internal Connector Type: SAS/SATA Plug Receptacle External Reference Designator: SATA Cable External Connector Type: None Port Type: Other Handle 0x0018, DMI type 8, 9 bytes Port Connector Information Internal Reference Designator: J4A1 Internal Connector Type: None External Reference Designator: VGA External Connector Type: DB-15 female Port Type: Video Port Handle 0x0019, DMI type 8, 9 bytes Port Connector Information Internal Reference Designator: J1A1 Internal Connector Type: None External Reference Designator: Display Port External Connector Type: Other Port Type: Video Port Handle 0x001A, DMI type 8, 9 bytes Port Connector Information Internal Reference Designator: J3A1 Internal Connector Type: None External Reference Designator: Display Port External Connector Type: Other Port Type: Video Port Handle 0x001B, DMI type 8, 9 bytes Port Connector Information Internal Reference Designator: J9B4 Internal Connector Type: None External Reference Designator: Serial Port External Connector Type: DB-9 male Port Type: Serial Port 16550A Compatible Handle 0x001C, DMI type 8, 9 bytes Port Connector Information Internal Reference Designator: J2A1 Internal Connector Type: None External Reference Designator: HDMI External Connector Type: Other Port Type: Other Handle 0x001D, DMI type 9, 17 bytes System Slot Information Designation: J5C1 Type: x4 PCI Express x4 Current Usage: Available Length: Other ID: 0 Characteristics: PME signal is supported Hot-plug devices are supported SMBus signal is supported Bus Address: 0000:00:00.0 Handle 0x001E, DMI type 9, 17 bytes System Slot Information Designation: J4B3 Type: x1 PCI Express x1 Current Usage: Available Length: Other ID: 0 Characteristics: PME signal is supported Hot-plug devices are supported SMBus signal is supported Bus Address: 0000:00:00.0 Handle 0x001F, DMI type 9, 17 bytes System Slot Information Designation: J6C1 Type: x1 PCI Express x1 Current Usage: Available Length: Other ID: 0 Characteristics: PME signal is supported Hot-plug devices are supported SMBus signal is supported Bus Address: 0000:00:00.0 Handle 0x0020, DMI type 9, 17 bytes System Slot Information Designation: J6D1 Type: x1 PCI Express x1 Current Usage: Available Length: Other ID: 0 Characteristics: PME signal is supported Hot-plug devices are supported SMBus signal is supported Bus Address: 0000:00:00.0 Handle 0x0021, DMI type 11, 5 bytes OEM Strings String 1: Insyde Chipset Handle 0x0022, DMI type 12, 5 bytes System Configuration Options Option 1: String1 for Type12 Equipment Manufacturer Option 2: String2 for Type12 Equipment Manufacturer Option 3: String3 for Type12 Equipment Manufacturer Option 4: String4 for Type12 Equipment Manufacturer Handle 0x0023, DMI type 13, 22 bytes BIOS Language Information Language Description Format: Long Installable Languages: 4 en|US|iso8859-1 fr|CA|iso8859-1 zh|TW|unicode ja|JP|unicode Currently Installed Language: en|US|iso8859-1 Handle 0x0024, DMI type 15, 29 bytes System Event Log Area Length: 0 bytes Header Start Offset: 0x0000 Header Length: 32 bytes Data Start Offset: 0x0020 Access Method: General-purpose non-volatile data functions Access Address: 0x0000 Status: Valid, Not Full Change Token: 0x12345678 Header Format: OEM-specific Supported Log Type Descriptors: 3 Descriptor 1: POST memory resize Data Format 1: None Descriptor 2: POST error Data Format 2: POST results bitmap Descriptor 3: Log area reset/cleared Data Format 3: None Handle 0x0025, DMI type 16, 15 bytes Physical Memory Array Location: System Board Or Motherboard Use: System Memory Error Correction Type: None Maximum Capacity: 8 GB Error Information Handle: No Error Number Of Devices: 2 Handle 0x0026, DMI type 17, 34 bytes Memory Device Array Handle: 0x0025 Error Information Handle: No Error Total Width: 64 bits Data Width: 64 bits Size: 4096 MB Form Factor: SODIMM Set: None Locator: DIMM0 Bank Locator: BANK 0 Type: DDR3 Type Detail: Synchronous Speed: 1066 MT/s Manufacturer: 00 Serial Number: 00000000 Asset Tag: Unknown Part Number: Rank: Unknown Configured Memory Speed: 1066 MT/s Handle 0x0027, DMI type 17, 34 bytes Memory Device Array Handle: 0x0025 Error Information Handle: No Error Total Width: Unknown Data Width: Unknown Size: No Module Installed Form Factor: SODIMM Set: None Locator: DIMM1 Bank Locator: BANK 1 Type: Unknown Type Detail: None Speed: Unknown Manufacturer: Not Specified Serial Number: Not Specified Asset Tag: Unknown Part Number: Not Specified Rank: Unknown Configured Memory Speed: Unknown Handle 0x0028, DMI type 19, 15 bytes Memory Array Mapped Address Starting Address: 0x00000000000 Ending Address: 0x000FFFFFFFF Range Size: 4 GB Physical Array Handle: 0x0025 Partition Width: 2 Handle 0x0029, DMI type 20, 19 bytes Memory Device Mapped Address Starting Address: 0x00000000000 Ending Address: 0x000FFFFFFFF Range Size: 4 GB Physical Device Handle: 0x0026 Memory Array Mapped Address Handle: 0x0028 Partition Row Position: Unknown Handle 0x002A, DMI type 21, 7 bytes Built-in Pointing Device Type: Touch Pad Interface: PS/2 Buttons: 2 Handle 0x002B, DMI type 22, 26 bytes Portable Battery Location: I2C2 Manufacturer: Intel SR 1 Manufacture Date: Date Serial Number: 123456789 Name: SR Real Battery Chemistry: Lithium Ion Design Capacity: 0 mWh Design Voltage: 3750 mV SBDS Version: CRB Battery 0 Maximum Error: Unknown OEM-specific Information: 0x00000000 Handle 0x002C, DMI type 26, 22 bytes Voltage Probe Description: Voltage Probe Description. Location: <OUT OF SPEC> Status: <OUT OF SPEC> Maximum Value: Unknown Minimum Value: Unknown Resolution: Unknown Tolerance: Unknown Accuracy: Unknown OEM-specific Information: 0x00000000 Nominal Value: Unknown Handle 0x002D, DMI type 27, 15 bytes Cooling Device Type: Chip Fan Status: OK OEM-specific Information: 0x00000000 Nominal Speed: Unknown Or Non-rotating Description: Cooling Device Description. Handle 0x002E, DMI type 28, 22 bytes Temperature Probe Description: Temperature Probe Description. Location: Unknown Status: Unknown Maximum Value: Unknown Minimum Value: Unknown Resolution: Unknown Tolerance: Unknown Accuracy: Unknown OEM-specific Information: 0x00000000 Nominal Value: Unknown Handle 0x002F, DMI type 32, 11 bytes System Boot Information Status: No errors detected Handle 0x0030, DMI type 39, 22 bytes System Power Supply Location: OEM_Define0 Name: OEM_Define1 Manufacturer: OEM_Define2 Serial Number: OEM_Define3 Asset Tag: OEM_Define4 Model Part Number: OEM_Define5 Revision: OEM_Define6 Max Power Capacity: 75 W Status: Present, OK Type: Regulator Input Voltage Range Switching: Auto-switch Plugged: No Hot Replaceable: No Input Voltage Probe Handle: 0x002C Cooling Device Handle: 0x002D Handle 0x0031, DMI type 41, 11 bytes Onboard Device Reference Designation: IGD Type: Video Status: Disabled Type Instance: 1 Bus Address: 0000:00:02.0 Handle 0xFEFF, DMI type 127, 4 bytes End Of Table ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue 2019-04-18 13:26 ` Semyon Verchenko @ 2019-04-18 15:12 ` Hans de Goede 2019-04-22 10:20 ` Semyon Verchenko 0 siblings, 1 reply; 24+ messages in thread From: Hans de Goede @ 2019-04-18 15:12 UTC (permalink / raw) To: Semyon Verchenko, Andy Shevchenko Cc: David Müller, Fujinaka, Todd, Kirsher, Jeffrey T, e1000-devel, Stephen Boyd, linux-clk, Michael Turquette, Stephen Boyd [-- Attachment #1: Type: text/plain, Size: 742 bytes --] Hi Semyon, On 18-04-19 15:26, Semyon Verchenko wrote: > > On 18.04.2019 16:09, Hans de Goede wrote: >> Hi, >> >> On 09-04-19 17:31, Andy Shevchenko wrote: <snip> >> Ok. >> >> Семен Верченко, this means that we are going to need DMI info from >> the board in question. I thought we already had that, but I now see that >> you original report did not have that a >> >> Please run as root: >> >> dmidecode &> dmidecode.log >> >> And then reply to this email with the generated dmidecode.log file >> attached. Once I have that file I can prepare a patch fixing this. Thank you for the DMI decode. Attached are 3 patches, can you please test if adding those 3 patches to your kernel fixes the problem? Thanks & Regards, Hans [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-clk-x86-Add-system-specific-quirk-to-mark-clocks-as-.patch --] [-- Type: text/x-patch; name="0001-clk-x86-Add-system-specific-quirk-to-mark-clocks-as-.patch", Size: 4991 bytes --] From 7c2e07130090ae001a97a6b65597830d6815e93e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20M=C3=BCller?= <dave.mueller@gmx.ch> Date: Mon, 8 Apr 2019 15:33:54 +0200 Subject: [PATCH] clk: x86: Add system specific quirk to mark clocks as critical MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since commit 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL"), the pmc_plt_clocks of the Bay Trail SoC are unconditionally gated off. Unfortunately this will break systems where these clocks are used for external purposes beyond the kernel's knowledge. Fix it by implementing a system specific quirk to mark the necessary pmc_plt_clks as critical. Fixes: 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL") Signed-off-by: David Müller <dave.mueller@gmx.ch> Signed-off-by: Hans de Goede <hdegoede@redhat.com> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> Signed-off-by: Stephen Boyd <sboyd@kernel.org> --- drivers/clk/x86/clk-pmc-atom.c | 14 ++++++++++--- drivers/platform/x86/pmc_atom.c | 21 +++++++++++++++++++ .../linux/platform_data/x86/clk-pmc-atom.h | 3 +++ 3 files changed, 35 insertions(+), 3 deletions(-) diff --git a/drivers/clk/x86/clk-pmc-atom.c b/drivers/clk/x86/clk-pmc-atom.c index d977193842df..19174835693b 100644 --- a/drivers/clk/x86/clk-pmc-atom.c +++ b/drivers/clk/x86/clk-pmc-atom.c @@ -165,7 +165,7 @@ static const struct clk_ops plt_clk_ops = { }; static struct clk_plt *plt_clk_register(struct platform_device *pdev, int id, - void __iomem *base, + const struct pmc_clk_data *pmc_data, const char **parent_names, int num_parents) { @@ -184,9 +184,17 @@ static struct clk_plt *plt_clk_register(struct platform_device *pdev, int id, init.num_parents = num_parents; pclk->hw.init = &init; - pclk->reg = base + PMC_CLK_CTL_OFFSET + id * PMC_CLK_CTL_SIZE; + pclk->reg = pmc_data->base + PMC_CLK_CTL_OFFSET + id * PMC_CLK_CTL_SIZE; spin_lock_init(&pclk->lock); + /* + * On some systems, the pmc_plt_clocks already enabled by the + * firmware are being marked as critical to avoid them being + * gated by the clock framework. + */ + if (pmc_data->critical && plt_clk_is_enabled(&pclk->hw)) + init.flags |= CLK_IS_CRITICAL; + ret = devm_clk_hw_register(&pdev->dev, &pclk->hw); if (ret) { pclk = ERR_PTR(ret); @@ -332,7 +340,7 @@ static int plt_clk_probe(struct platform_device *pdev) return PTR_ERR(parent_names); for (i = 0; i < PMC_CLK_NUM; i++) { - data->clks[i] = plt_clk_register(pdev, i, pmc_data->base, + data->clks[i] = plt_clk_register(pdev, i, pmc_data, parent_names, data->nparents); if (IS_ERR(data->clks[i])) { err = PTR_ERR(data->clks[i]); diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c index 8f018b3f3cd4..eaec2d306481 100644 --- a/drivers/platform/x86/pmc_atom.c +++ b/drivers/platform/x86/pmc_atom.c @@ -17,6 +17,7 @@ #include <linux/debugfs.h> #include <linux/device.h> +#include <linux/dmi.h> #include <linux/init.h> #include <linux/io.h> #include <linux/platform_data/x86/clk-pmc-atom.h> @@ -391,11 +392,27 @@ static int pmc_dbgfs_register(struct pmc_dev *pmc) } #endif /* CONFIG_DEBUG_FS */ +/* + * Some systems need one or more of their pmc_plt_clks to be + * marked as critical. + */ +static const struct dmi_system_id critclk_systems[] __initconst = { + { + .ident = "MPL CEC1x", + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "MPL AG"), + DMI_MATCH(DMI_PRODUCT_NAME, "CEC10 Family"), + }, + }, + { /*sentinel*/ } +}; + static int pmc_setup_clks(struct pci_dev *pdev, void __iomem *pmc_regmap, const struct pmc_data *pmc_data) { struct platform_device *clkdev; struct pmc_clk_data *clk_data; + const struct dmi_system_id *d = dmi_first_match(critclk_systems); clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL); if (!clk_data) @@ -403,6 +420,10 @@ static int pmc_setup_clks(struct pci_dev *pdev, void __iomem *pmc_regmap, clk_data->base = pmc_regmap; /* offset is added by client */ clk_data->clks = pmc_data->clks; + if (d) { + clk_data->critical = true; + pr_info("%s critclks quirk enabled\n", d->ident); + } clkdev = platform_device_register_data(&pdev->dev, "clk-pmc-atom", PLATFORM_DEVID_NONE, diff --git a/include/linux/platform_data/x86/clk-pmc-atom.h b/include/linux/platform_data/x86/clk-pmc-atom.h index 3ab892208343..7a37ac27d0fb 100644 --- a/include/linux/platform_data/x86/clk-pmc-atom.h +++ b/include/linux/platform_data/x86/clk-pmc-atom.h @@ -35,10 +35,13 @@ struct pmc_clk { * * @base: PMC clock register base offset * @clks: pointer to set of registered clocks, typically 0..5 + * @critical: flag to indicate if firmware enabled pmc_plt_clks + * should be marked as critial or not */ struct pmc_clk_data { void __iomem *base; const struct pmc_clk *clks; + bool critical; }; #endif /* __PLATFORM_DATA_X86_CLK_PMC_ATOM_H */ -- 2.21.0 [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #3: 0002-platform-x86-pmc_atom-Drop-__initconst-on-dmi-table.patch --] [-- Type: text/x-patch; name="0002-platform-x86-pmc_atom-Drop-__initconst-on-dmi-table.patch", Size: 1354 bytes --] From b995dcca7cf12f208cfd95fd9d5768dca7cccec7 Mon Sep 17 00:00:00 2001 From: Stephen Boyd <sboyd@kernel.org> Date: Thu, 11 Apr 2019 10:22:43 -0700 Subject: [PATCH] platform/x86: pmc_atom: Drop __initconst on dmi table MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It's used by probe and that isn't an init function. Drop this so that we don't get a section mismatch. Reported-by: kbuild test robot <lkp@intel.com> Cc: David Müller <dave.mueller@gmx.ch> Cc: Hans de Goede <hdegoede@redhat.com> Cc: Andy Shevchenko <andy.shevchenko@gmail.com> Fixes: 7c2e07130090 ("clk: x86: Add system specific quirk to mark clocks as critical") Signed-off-by: Stephen Boyd <sboyd@kernel.org> --- drivers/platform/x86/pmc_atom.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c index eaec2d306481..c7039f52ad51 100644 --- a/drivers/platform/x86/pmc_atom.c +++ b/drivers/platform/x86/pmc_atom.c @@ -396,7 +396,7 @@ static int pmc_dbgfs_register(struct pmc_dev *pmc) * Some systems need one or more of their pmc_plt_clks to be * marked as critical. */ -static const struct dmi_system_id critclk_systems[] __initconst = { +static const struct dmi_system_id critclk_systems[] = { { .ident = "MPL CEC1x", .matches = { -- 2.21.0 [-- Attachment #4: 0003-platform-x86-Add-Lex-3I380D-industrial-PC-to-critclk.patch --] [-- Type: text/x-patch, Size: 1480 bytes --] From dcf861e0a71906903189baed4817a90e37dc78cc Mon Sep 17 00:00:00 2001 From: Hans de Goede <hdegoede@redhat.com> Date: Thu, 18 Apr 2019 17:04:10 +0200 Subject: [PATCH] platform: x86: Add Lex 3I380D industrial PC to critclk_systems DMI table The Lex 3I380D industrial PC has 4 ethernet controllers on board which need pmc_plt_clk0 - 3 to function, add it to the critclk_systems DMI table, so that drivers/clk/x86/clk-pmc-atom.c will mark the clocks as CLK_CRITICAL and they will not get turned off. Reported-by: Semyon Verchenko <semverchenko@factor-ts.ru> Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/platform/x86/pmc_atom.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c index 3a635ea09b8a..2910845b7cdd 100644 --- a/drivers/platform/x86/pmc_atom.c +++ b/drivers/platform/x86/pmc_atom.c @@ -407,12 +407,21 @@ static int pmc_dbgfs_register(struct pmc_dev *pmc) */ static const struct dmi_system_id critclk_systems[] = { { + /* pmc_plt_clk0 is used for an external HSIC USB HUB */ .ident = "MPL CEC1x", .matches = { DMI_MATCH(DMI_SYS_VENDOR, "MPL AG"), DMI_MATCH(DMI_PRODUCT_NAME, "CEC10 Family"), }, }, + { + /* pmc_plt_clk0 - 3 are used for the 4 ethernet controllers */ + .ident = "Lex 3I380D", + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "Lex BayTrail"), + DMI_MATCH(DMI_PRODUCT_NAME, "3I380D"), + }, + }, { /*sentinel*/ } }; -- 2.21.0 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue 2019-04-18 15:12 ` Hans de Goede @ 2019-04-22 10:20 ` Semyon Verchenko 2019-04-29 15:02 ` Hans de Goede 0 siblings, 1 reply; 24+ messages in thread From: Semyon Verchenko @ 2019-04-22 10:20 UTC (permalink / raw) To: Hans de Goede, Andy Shevchenko Cc: David Müller, Fujinaka, Todd, Kirsher, Jeffrey T, e1000-devel, Stephen Boyd, linux-clk, Michael Turquette, Stephen Boyd On 18.04.2019 18:12, Hans de Goede wrote: > Hi Semyon, > > On 18-04-19 15:26, Semyon Verchenko wrote: >> >> On 18.04.2019 16:09, Hans de Goede wrote: >>> Hi, >>> >>> On 09-04-19 17:31, Andy Shevchenko wrote: > > <snip> > >>> Ok. >>> >>> Семен Верченко, this means that we are going to need DMI info from >>> the board in question. I thought we already had that, but I now see >>> that >>> you original report did not have that a >>> >>> Please run as root: >>> >>> dmidecode &> dmidecode.log >>> >>> And then reply to this email with the generated dmidecode.log file >>> attached. Once I have that file I can prepare a patch fixing this. > > Thank you for the DMI decode. > > Attached are 3 patches, can you please test if adding those 3 patches > to your kernel fixes the problem? > > Thanks & Regards, > > Hans > Hi Hans, It seems that these patches fix the problem (at least interfaces are visible through ip addr and it's possible to ping something from them). Thanks for fixing this issue. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue 2019-04-22 10:20 ` Semyon Verchenko @ 2019-04-29 15:02 ` Hans de Goede 0 siblings, 0 replies; 24+ messages in thread From: Hans de Goede @ 2019-04-29 15:02 UTC (permalink / raw) To: Semyon Verchenko, Andy Shevchenko Cc: David Müller, Fujinaka, Todd, Kirsher, Jeffrey T, e1000-devel, Stephen Boyd, linux-clk, Michael Turquette, Stephen Boyd Hi, On 22-04-19 12:20, Semyon Verchenko wrote: > > On 18.04.2019 18:12, Hans de Goede wrote: >> Hi Semyon, >> >> On 18-04-19 15:26, Semyon Verchenko wrote: >>> >>> On 18.04.2019 16:09, Hans de Goede wrote: >>>> Hi, >>>> >>>> On 09-04-19 17:31, Andy Shevchenko wrote: >> >> <snip> >> >>>> Ok. >>>> >>>> Семен Верченко, this means that we are going to need DMI info from >>>> the board in question. I thought we already had that, but I now see that >>>> you original report did not have that a >>>> >>>> Please run as root: >>>> >>>> dmidecode &> dmidecode.log >>>> >>>> And then reply to this email with the generated dmidecode.log file >>>> attached. Once I have that file I can prepare a patch fixing this. >> >> Thank you for the DMI decode. >> >> Attached are 3 patches, can you please test if adding those 3 patches >> to your kernel fixes the problem? >> >> Thanks & Regards, >> >> Hans >> > Hi Hans, > It seems that these patches fix the problem (at least interfaces are visible through ip addr and it's possible to ping something from them). > > Thanks for fixing this issue. Thank you for testing the fix, I've submitted the patch fixing this upstream. Regards, Hans ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue 2019-03-27 16:47 ` Andy Shevchenko 2019-03-27 17:31 ` David Müller @ 2019-03-28 14:48 ` Hans de Goede 1 sibling, 0 replies; 24+ messages in thread From: Hans de Goede @ 2019-03-28 14:48 UTC (permalink / raw) To: Andy Shevchenko Cc: Fujinaka, Todd, Semyon Verchenko, Kirsher, Jeffrey T, e1000-devel, Stephen Boyd, David Müller, linux-clk, Michael Turquette, Stephen Boyd Hi, On 27-03-19 17:47, Andy Shevchenko wrote: > On Wed, Mar 27, 2019 at 05:02:31PM +0100, Hans de Goede wrote: >> Hi, >> >> On 3/25/19 9:12 PM, Fujinaka, Todd wrote: >>> This is going to take a bit of time to see what we need to do. The attachments were stripped but I think just figuring out what they had to change in the Realtek driver will tell us what we need to know. >> >> I'm not sure fixing this in the ethernet driver is the best way to go, >> the board in question is an embedded PC and I've also recently received >> a bug report about a similar problem with the consumer not requesting >> the pmc clocks causing a HSIC usb hub to not work. >> >> I think that it might be better to restore the CLK_IS_CRITICAL workaround >> for this, but then only on select boards, based on DMI matching. >> >> I've added a bunch of relevant people / lists to the Cc. >> >> Andy, Stephen, what is your take on this ? > > I'm afraid I forgot all details about that (semi-)famous issue. Though, looking > into your patch against r8169 and taking into account DT practice, it would be > not bad to fix a driver, we have by the way devm_clk_get_optional() now, so, it > would be not a big deal. This assumes that the machine with the igb ethernet problem is using the same pmc clk as the other device I fixed and that it is using one and the same clock for all 4 of its ethernet controllers. IMHO the not controlling of the clock as an ACPI resource on Bay Trail models as is normal on x86 really is a firmware bug (shared by the entire Bay Trail generation). I'm not in favor of adding workarounds to drivers all over the place because of this. And in case of the problem with David Müller's system, the problem is that the clock is needed for an external USB hub connected via HSIC. The USB subsystem assumes that HUBs will just work without needing to request resources for them and in this case there really is no good place to do the devm_clk_get_optional(). So I believe that we are going to need the DMI based approach for David's case anyway, at which point we might just as well also fix the igb problem that way, at least until more boards using the igb driver and showing the same problem pop up. The reasons I patched the r8169 driver for this are: 1) In principle I agree that this is the right thing to do (but see above) 2) I expect more Cherry Trail based devices (yes this is a Cherry Trail device, so here it really really is a firmware bug) to use the r8169 (cheap SoC + cheap ethernet chip) 3) This is a laptop where being able to reach S0i3 is important, which the CLK_IS_CRITICAL workaround will break >> I'm myself starting to believe the DMI based applying of the >> CLK_IS_CRITICAL workaround is the best solution here. > > DMI quirk table tends to grow in mysterious ways. I would prefer in this case > logical solution — if platform has an optional clock, then use it. I'm afraid that otherwise instead of a growing DMI table in a single place we will get extra coded added for niche cases to a bunch of different drivers, in which case I prefer the "central" DMI quirk database. Regards, Hans ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2019-04-29 15:02 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <1c433bef-055e-2ac3-990c-325aa2d3899e@factor-ts.ru> [not found] ` <9B4A1B1917080E46B64F07F2989DADD69AF4DDE7@ORSMSX115.amr.corp.intel.com> 2019-03-27 16:02 ` [E1000-devel] igb driver with Intel Atom Bay Trail issue Hans de Goede 2019-03-27 16:47 ` Andy Shevchenko 2019-03-27 17:31 ` David Müller 2019-03-27 19:19 ` Andy Shevchenko 2019-03-28 14:35 ` David Müller 2019-03-28 14:58 ` Andy Shevchenko 2019-03-28 15:01 ` Hans de Goede 2019-03-28 15:24 ` Andy Shevchenko 2019-03-28 15:32 ` Hans de Goede 2019-03-28 15:49 ` Andy Shevchenko 2019-03-29 4:46 ` Hisashi T Fujinaka 2019-03-29 12:32 ` Hans de Goede 2019-03-29 12:30 ` Hans de Goede 2019-03-29 13:59 ` Семен Верченко 2019-03-29 15:53 ` Hans de Goede 2019-04-04 14:43 ` Hans de Goede [not found] ` <20190408172111.GX9224@smile.fi.intel.com> 2019-04-08 18:43 ` Hans de Goede 2019-04-09 15:31 ` Andy Shevchenko 2019-04-18 13:09 ` Hans de Goede 2019-04-18 13:26 ` Semyon Verchenko 2019-04-18 15:12 ` Hans de Goede 2019-04-22 10:20 ` Semyon Verchenko 2019-04-29 15:02 ` Hans de Goede 2019-03-28 14:48 ` Hans de Goede
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).