Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue

Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue

[Hans de Goede]

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
> 

Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue

[Andy Shevchenko]

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

Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue

[David Müller]

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

Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue

[Andy Shevchenko]

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

Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue

[David Müller]

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

Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue

[Andy Shevchenko]

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

Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue

[Hans de Goede]

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

Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue

[Andy Shevchenko]

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

Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue

[Hans de Goede]

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

Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue

[Andy Shevchenko]

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

Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue

[Hisashi T Fujinaka]

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>

Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue

[Hans de Goede]

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

Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue

[Hans de Goede]

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

Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue

[Семен Верченко]

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?

Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue

[Hans de Goede]

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

Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue

[Hans de Goede]

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

Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue

[Hans de Goede]

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

Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue

[Andy Shevchenko]

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

Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue

[Hans de Goede]

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

Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue

[Semyon Verchenko]

[-- 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

Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue

[Hans de Goede]

[-- 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

Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue

[Semyon Verchenko]

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.

Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue

[Hans de Goede]

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

Re: [E1000-devel] igb driver with Intel Atom Bay Trail issue

[Hans de Goede]

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