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

[Hans de G. <hde...@re...>]

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