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.
Signed-off-by: Jeremy Linton <[email protected]>
---
drivers/net/ethernet/broadcom/genet/bcmgenet.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index dbf96fc96689..115977e2edef 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -3572,7 +3572,7 @@ static int bcmgenet_probe(struct platform_device *pdev)
priv->clk = devm_clk_get(&priv->pdev->dev, "enet");
if (IS_ERR(priv->clk)) {
- dev_warn(&priv->pdev->dev, "failed to get enet clock\n");
+ dev_dbg(&priv->pdev->dev, "failed to get enet clock\n");
priv->clk = NULL;
}
@@ -3588,13 +3588,13 @@ static int bcmgenet_probe(struct platform_device *pdev)
priv->clk_wol = devm_clk_get(&priv->pdev->dev, "enet-wol");
if (IS_ERR(priv->clk_wol)) {
- dev_warn(&priv->pdev->dev, "failed to get enet-wol clock\n");
+ dev_dbg(&priv->pdev->dev, "failed to get enet-wol clock\n");
priv->clk_wol = NULL;
}
priv->clk_eee = devm_clk_get(&priv->pdev->dev, "enet-eee");
if (IS_ERR(priv->clk_eee)) {
- dev_warn(&priv->pdev->dev, "failed to get enet-eee clock\n");
+ dev_dbg(&priv->pdev->dev, "failed to get enet-eee clock\n");
priv->clk_eee = NULL;
}
--
2.24.1
On 1/31/2020 11:46 PM, Jeremy Linton wrote:
> 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.
>
> Signed-off-by: Jeremy Linton <[email protected]>
Acked-by: Florian Fainelli <[email protected]>
--
Florian
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.
This is what i got so far:
diff --git a/arch/arm/boot/dts/bcm2711.dtsi b/arch/arm/boot/dts/bcm2711.dtsi
index 961bed8..d4ff370 100644
--- a/arch/arm/boot/dts/bcm2711.dtsi
+++ b/arch/arm/boot/dts/bcm2711.dtsi
@@ -338,6 +338,8 @@
reg = <0x0 0x7d580000 0x10000>;
#address-cells = <0x1>;
#size-cells = <0x1>;
+ clocks = <&clocks BCM2711_CLOCK_GENET250>;
+ clock-names = "enet";
interrupts = <GIC_SPI 157 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 158 IRQ_TYPE_LEVEL_HIGH>;
status = "disabled";
diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index ded13cc..627f1b1 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -116,6 +116,10 @@
#define CM_EMMCDIV 0x1c4
#define CM_EMMC2CTL 0x1d0
#define CM_EMMC2DIV 0x1d4
+#define CM_GENET250CTL 0x1e8
+#define CM_GENET250DIV 0x1ec
+#define CM_GENET125CTL 0x210
+#define CM_GENET125DIV 0x214
/* General bits for the CM_*CTL regs */
# define CM_ENABLE BIT(4)
@@ -2021,6 +2025,25 @@ static const struct bcm2835_clk_desc
clk_desc_array[] = {
.frac_bits = 8,
.tcnt_mux = 42),
+ /* GENET clocks (only available for BCM2711) */
+ [BCM2711_CLOCK_GENET250] = REGISTER_PER_CLK(
+ SOC_BCM2711,
+ .name = "genet250",
+ .ctl_reg = CM_GENET250CTL,
+ .div_reg = CM_GENET250DIV,
+ .int_bits = 4,
+ .frac_bits = 8,
+ .tcnt_mux = 45),
+
+ [BCM2711_CLOCK_GENET125] = REGISTER_PER_CLK(
+ SOC_BCM2711,
+ .name = "genet125",
+ .ctl_reg = CM_GENET125CTL,
+ .div_reg = CM_GENET125DIV,
+ .int_bits = 4,
+ .frac_bits = 8,
+ .tcnt_mux = 50),
+
/* General purpose (GPIO) clocks */
[BCM2835_CLOCK_GP0] = REGISTER_PER_CLK(
SOC_ALL,
diff --git a/include/dt-bindings/clock/bcm2835.h
b/include/dt-bindings/clock/bcm2835.h
index b60c0343..fca65ab 100644
--- a/include/dt-bindings/clock/bcm2835.h
+++ b/include/dt-bindings/clock/bcm2835.h
@@ -60,3 +60,5 @@
#define BCM2835_CLOCK_DSI1P 50
#define BCM2711_CLOCK_EMMC2 51
+#define BCM2711_CLOCK_GENET250 52
+#define BCM2711_CLOCK_GENET125 53
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:
BTW: For DT, is part of the problem here that the videocore mailbox has
a clock management method? For ACPI one of the paths of investigation is
to write AML which just interfaces to that mailbox interface for clock
control here. (there is also SCMII to be considered).
>
> diff --git a/arch/arm/boot/dts/bcm2711.dtsi b/arch/arm/boot/dts/bcm2711.dtsi
> index 961bed8..d4ff370 100644
> --- a/arch/arm/boot/dts/bcm2711.dtsi
> +++ b/arch/arm/boot/dts/bcm2711.dtsi
> @@ -338,6 +338,8 @@
> reg = <0x0 0x7d580000 0x10000>;
> #address-cells = <0x1>;
> #size-cells = <0x1>;
> + clocks = <&clocks BCM2711_CLOCK_GENET250>;
> + clock-names = "enet";
> interrupts = <GIC_SPI 157 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 158 IRQ_TYPE_LEVEL_HIGH>;
> status = "disabled";
> diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
> index ded13cc..627f1b1 100644
> --- a/drivers/clk/bcm/clk-bcm2835.c
> +++ b/drivers/clk/bcm/clk-bcm2835.c
> @@ -116,6 +116,10 @@
> #define CM_EMMCDIV 0x1c4
> #define CM_EMMC2CTL 0x1d0
> #define CM_EMMC2DIV 0x1d4
> +#define CM_GENET250CTL 0x1e8
> +#define CM_GENET250DIV 0x1ec
> +#define CM_GENET125CTL 0x210
> +#define CM_GENET125DIV 0x214
>
> /* General bits for the CM_*CTL regs */
> # define CM_ENABLE BIT(4)
> @@ -2021,6 +2025,25 @@ static const struct bcm2835_clk_desc
> clk_desc_array[] = {
> .frac_bits = 8,
> .tcnt_mux = 42),
>
> + /* GENET clocks (only available for BCM2711) */
> + [BCM2711_CLOCK_GENET250] = REGISTER_PER_CLK(
> + SOC_BCM2711,
> + .name = "genet250",
> + .ctl_reg = CM_GENET250CTL,
> + .div_reg = CM_GENET250DIV,
> + .int_bits = 4,
> + .frac_bits = 8,
> + .tcnt_mux = 45),
> +
> + [BCM2711_CLOCK_GENET125] = REGISTER_PER_CLK(
> + SOC_BCM2711,
> + .name = "genet125",
> + .ctl_reg = CM_GENET125CTL,
> + .div_reg = CM_GENET125DIV,
> + .int_bits = 4,
> + .frac_bits = 8,
> + .tcnt_mux = 50),
> +
> /* General purpose (GPIO) clocks */
> [BCM2835_CLOCK_GP0] = REGISTER_PER_CLK(
> SOC_ALL,
> diff --git a/include/dt-bindings/clock/bcm2835.h
> b/include/dt-bindings/clock/bcm2835.h
> index b60c0343..fca65ab 100644
> --- a/include/dt-bindings/clock/bcm2835.h
> +++ b/include/dt-bindings/clock/bcm2835.h
> @@ -60,3 +60,5 @@
> #define BCM2835_CLOCK_DSI1P 50
>
> #define BCM2711_CLOCK_EMMC2 51
> +#define BCM2711_CLOCK_GENET250 52
> +#define BCM2711_CLOCK_GENET125 53
>
>
Hi,
BTW the patch looks good to me too:
Reviewed-by: Nicolas Saenz Julienne <[email protected]>
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?
> BTW: For DT, is part of the problem here that the videocore mailbox has
> a clock management method?
I don't think it'll be the case for these clocks. We try to only use the
mailbox interface if access to the clock is shared with videocore's firmware.
The only example for now is 'pllb' which drives the CPU. See clk-raspberrypi.c
for the firmware part and clk-bcm2835.c for the rest.
Note that the firmware interface has some shortcomings, it isn't fine grained
nor provides a full clock tree to work with, also some clock changes, from
videocore's point of view, might change multiple plls behind your back. See for
example the ARM clock, at offset 0x3[1]: if you don't explicitly disable turbo
mode, it'll change both pllb and pllc. Affecting a whole lot of peripherals.
In an Ideal world I'd love to see them implement ARM's SCMI[2]. It would make
our lives easier.
> For ACPI one of the paths of investigation is to write AML which just
> interfaces to that mailbox interface for clock control here. (there is also
> SCMII to be considered).
As we're on the topic of integrating the mailbox interfaces with ACPI, have you
looked at VCHIQ in the staging directory? It serves as an interface to
videocore for the camera, HDMI audio and video codec drivers. It ultimately
depends on the mailbox interface mentioned above. It might be interesting for
you to look into it before writing the AML interface to the mailbox.
Regards,
Nicolas
[1] https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface
[2] https://github.com/raspberrypi/firmware/issues/1139
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 <[email protected]>
>
> 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.
Best regards
Stefan
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 <[email protected]>
>>
>> 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
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 <[email protected]>
>>>
>>> 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.