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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <34aba1d9-5cad-0fee-038d-c5f3bfc9ed30@arm.com>
Date:   Sat, 1 Feb 2020 13:27:12 -0600
From:   Jeremy Linton <jeremy.linton@....com>
To:     Stefan Wahren <wahrenst@....net>, netdev@...r.kernel.org
Cc:     opendmb@...il.com, f.fainelli@...il.com, davem@...emloft.net,
        bcm-kernel-feedback-list@...adcom.com,
        linux-kernel@...r.kernel.org, andrew@...n.ch, hkallweit1@...il.com,
        Nicolas Saenz Julienne <nsaenzjulienne@...e.de>
Subject: Re: [PATCH 6/6] net: bcmgenet: reduce severity of missing clock
 warnings

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