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

On 2/3/20 11:08 AM, Stefan Wahren wrote:

Hi,

Am 03.02.20 um 19:36 schrieb Nicolas Saenz Julienne:

quoted

Hi,
BTW the patch looks good to me too:

Reviewed-by: Nicolas Saenz Julienne <redacted>

On Sat, 2020-02-01 at 13:27 -0600, Jeremy Linton wrote:

quoted

Hi,

First, thanks for looking at this!

On 2/1/20 10:44 AM, Stefan Wahren wrote:

quoted

Hi Jeremy,

[add Nicolas as BCM2835 maintainer]

Am 01.02.20 um 08:46 schrieb Jeremy Linton:

quoted

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.

quoted

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