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

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

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.
-- 
Florian