Re: [PATCH 6/6] net: bcmgenet: reduce severity of missing clock warnings

[Stefan Wahren]

Hi Florian,

Am 03.02.20 um 22:21 schrieb Florian Fainelli:
> On 2/3/20 11:08 AM, Stefan Wahren wrote:
>> Hi,
>>
>> Am 03.02.20 um 19:36 schrieb Nicolas Saenz Julienne:
>>> Hi,
>>> BTW the patch looks good to me too:
>>>
>>> Reviewed-by: Nicolas Saenz Julienne <nsaenzjulienne@xxxxxxx>
>>>
>>> On Sat, 2020-02-01 at 13:27 -0600, Jeremy Linton wrote:
>>>> Hi,
>>>>
>>>> First, thanks for looking at this!
>>>>
>>>> On 2/1/20 10:44 AM, Stefan Wahren wrote:
>>>>> Hi Jeremy,
>>>>>
>>>>> [add Nicolas as BCM2835 maintainer]
>>>>>
>>>>> Am 01.02.20 um 08:46 schrieb Jeremy Linton:
>>>>>> If one types "failed to get enet clock" or similar into google
>>>>>> there are ~370k hits. The vast majority are people debugging
>>>>>> problems unrelated to this adapter, or bragging about their
>>>>>> rpi's. Given that its not a fatal situation with common DT based
>>>>>> systems, lets reduce the severity so people aren't seeing failure
>>>>>> messages in everyday operation.
>>>>>>
>>>>> i'm fine with your patch, since the clocks are optional according to the
>>>>> binding. But instead of hiding of those warning, it would be better to
>>>>> fix the root cause (missing clocks). Unfortunately i don't have the
>>>>> necessary documentation, just some answers from the RPi guys.
>>>> The DT case just added to my ammunition here :)
>>>>
>>>> But really, I'm fixing an ACPI problem because the ACPI power management
>>>> methods are also responsible for managing the clocks. Which means if I
>>>> don't lower the severity (or otherwise tweak the code path) these errors
>>>> are going to happen on every ACPI boot.
>>>>
>>>>> This is what i got so far:
>>> Stefan, Apart from the lack of documentation (and maybe also time), is there
>>> any specific reason you didn't sent the genet clock patch yet? It should be OK
>>> functionally isn't it?
>> last time i tried to specify the both clocks as suggest by the binding
>> document (took genet125 for wol, not sure this is correct), but this
>> caused an abort on the BCM2711. In the lack of documentation i stopped
>> further investigations. As i saw that Jeremy send this patch, i wanted
>> to share my current results and retestet it with this version which
>> doesn't crash. I don't know the reason why both clocks should be
>> specified, but this patch should be acceptable since the RPi 4 doesn't
>> support wake on LAN.
> Your clock changes look correct, but there is also a CLKGEN register
> block which has separate clocks for the GENET controller, which lives at
> register offset 0x7d5e0048 and which has the following layout:
>
> bit 0: GENET_CLK_250_CLOCK_ENABLE
> bit 1: GENET_EEE_CLOCK_ENABLE
> bit 2: GENET_GISB_CLOCK_ENABLE
> bit 3: GENET_GMII_CLOCK_ENABLE
> bit 4: GENET_HFB_CLOCK_ENABLE
> bit 5: GENET_L2_INTR_CLOCK_ENABLE
> bit 6: GENET_SCB_CLOCK_ENABLE
> bit 7: GENET_UNIMAC_SYS_RX_CLOCK_ENABLE
> bit 8: GENET_UNIMAC_SYS_TX_CLOCK_ENABLE
>
> you will need all of those clocks turned on for normal operation minus
> EEE, unless EEE is desirable which is why it is a separate clock. Those
> clocks default to ON unless turned off, and the main gate that you
> control is probably enough.
so you suggest to add these clock gate(s) to the clk-bcm2835 or
introduce a "clk-genet" from DT perspective?
>
> The Pi4 could support Wake-on-LAN with appropriate VPU firmware changes,
> but I do not believe there is any interest in doing that. I would not
> "bend" the clock representation just so as to please the driver with its
> clocking needs.