linux-kernel - Re: [PATCH 1/4] arm64: dts: mediatek: mt8183: Fix systimer 13 MHz clock description

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: <5250d7d3-ff46-e08d-926c-4efd92390d88@collabora.com>
Date:   Thu, 1 Dec 2022 10:31:41 +0100
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
To:     Chen-Yu Tsai <wenst@...omium.org>,
        Matthias Brugger <matthias.bgg@...il.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Nícolas F . R . A . Prado 
        <nfraprado@...labora.com>
Subject: Re: [PATCH 1/4] arm64: dts: mediatek: mt8183: Fix systimer 13 MHz
 clock description

Il 01/12/22 09:42, Chen-Yu Tsai ha scritto:
> The systimer block derives its 13 MHz clock by dividing the main 26 MHz
> oscillator clock by 2 internally, not through the TOPCKGEN clock
> controller.
> 
> On the MT8183 this divider is set either by power-on-reset or by the
> bootloader. The bootloader may then make the divider unconfigurable to,
> but can be read out by, the operating system.
> 
> Making the systimer block take the 26 MHz clock directly requires
> changing the implementations. As an ABI compatible fix, change the
> input clock of the systimer block a fixed factor divide-by-2 clock
> that takes the 26 MHz oscillator as its input.
> 
> Fixes: 5bc8e2875ffb ("arm64: dts: mt8183: add systimer0 device node")
> Signed-off-by: Chen-Yu Tsai <wenst@...omium.org>

I generally not just like - but *love* - this change, I had that in my mind
for a couple of months now and forgot about it because reasons.

There's just one thing that, since we're doing this now, we can clarify (and
that's important to avoid questions like "why isn't this board-specific"):
the 26MHz clock "clk26m" oscillator that we're using for the system timers
is a SoC-provided clock, and its name is "SYSCLK" as in "System bus clock".

I know that your target is to describe how we get from 26M to 13M, but at
this point it may be worth it to use the right names to help preventing
confusion about that clock not being an external crystal on the board but
something internal to the SoC.

So, I propose:
1. Change `clk26m: oscillator` to `clk26m: sysclk` or `clk26m: sysclk-26m`;
2. Add the divider as `clk13m: sysclk-div2`.

What do you think?

Cheers,
Angelo

> ---
>   arch/arm64/boot/dts/mediatek/mt8183.dtsi | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> index 19ff1babc359..0cbbaebe1213 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> @@ -585,6 +585,15 @@ psci {
>   		method = "smc";
>   	};
>   
> +	clk13m: fixed-factor-clock-13m {
> +		compatible = "fixed-factor-clock";
> +		#clock-cells = <0>;
> +		clocks = <&clk26m>;
> +		clock-div = <2>;
> +		clock-mult = <1>;
> +		clock-output-names = "clk13m";
> +	};
> +
>   	clk26m: oscillator {
>   		compatible = "fixed-clock";
>   		#clock-cells = <0>;
> @@ -968,8 +977,7 @@ systimer: timer@...17000 {
>   				     "mediatek,mt6765-timer";
>   			reg = <0 0x10017000 0 0x1000>;
>   			interrupts = <GIC_SPI 200 IRQ_TYPE_LEVEL_HIGH>;
> -			clocks = <&topckgen CLK_TOP_CLK13M>;
> -			clock-names = "clk13m";
> +			clocks = <&clk13m>;
>   		};
>   
>   		iommu: iommu@...05000 {