linux-kernel - Re: [PATCH v6 2/3] arm64: dts: marvell: Add Armada 98DX2530 SoC and RD-AC5X board

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: <5ec00142-dc46-06e6-c991-c33394db614c@linaro.org>
Date:   Thu, 12 May 2022 16:45:24 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Chris Packham <Chris.Packham@...iedtelesis.co.nz>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "krzysztof.kozlowski+dt@...aro.org" 
        <krzysztof.kozlowski+dt@...aro.org>,
        "catalin.marinas@....com" <catalin.marinas@....com>,
        "will@...nel.org" <will@...nel.org>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "gregory.clement@...tlin.com" <gregory.clement@...tlin.com>,
        "sebastian.hesselbarth@...il.com" <sebastian.hesselbarth@...il.com>,
        "kostap@...vell.com" <kostap@...vell.com>,
        "robert.marko@...tura.hr" <robert.marko@...tura.hr>,
        Vadym Kochan <vadym.kochan@...ision.eu>
Cc:     "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v6 2/3] arm64: dts: marvell: Add Armada 98DX2530 SoC and
 RD-AC5X board

On 12/05/2022 01:49, Chris Packham wrote:
> 
>>> +		spi_clock: spi-clock {
>>> +			compatible = "fixed-clock";
>>> +			#clock-cells = <0>;
>>> +			clock-frequency = <200000000>;
>>> +		};
>> My questions about these clocks are still unanswered. Why do you define
>> fixed-clocks. Aren't these part of clock controller?
> 
> Not one that I have any information on. Marvell don't put it in their 
> customer facing documentation so I can only speculate. The 25MHz 
> oscillator goes into the chip, from there I guess that it is fed in some 
> fashion to both the CPU block (CnM in Marvell speak) and to the switch 
> block. Where exactly it gets divided into these individual peripheral 
> clocks I don't really know.

Hm, so it seems you do not have a proper clock-controller (or cannot
create one). OK then, but these are silly stubs, you know. :)

> 
>>> +	};
>>> +};
>>> diff --git a/arch/arm64/boot/dts/marvell/rd-ac5x.dts b/arch/arm64/boot/dts/marvell/rd-ac5x.dts
>>> new file mode 100644
>>> index 000000000000..7ac87413e023
>>> --- /dev/null
>>> +++ b/arch/arm64/boot/dts/marvell/rd-ac5x.dts
>>> @@ -0,0 +1,90 @@
>>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>>> +/*
>>> + * Device Tree For AC5X.
>>> + *
>>> + * Copyright (C) 2021 Marvell
>>> + *
>>> + */
>>> +/*
>>> + * Device Tree file for Marvell Alleycat 5X development board
>>> + * This board file supports the B configuration of the board
>>> + */
>>> +
>>> +/dts-v1/;
>>> +
>>> +#include "armada-98dx2530.dtsi"
>>> +
>>> +/ {
>>> +	model = "Marvell RD-AC5X Board";
>>> +	compatible = "marvell,ac5x", "marvell,ac5";
>>  From the bindings I understood ac5x is a SoC, not board. If ac5x is a
>> board, not a SoC, then compatible should be rather "marvell,rd-ac5x".
> 
> So If I understand the convention the full compatible would be:
> 
> compatible = "marvell,rd-ac5x", "marvell,ac5x", "marvell,ac5";

Yes, this looks correct.

> 
>>
>>> +
>>> +	memory@0 {
>>> +		device_type = "memory";
>>> +		reg = <0x2 0x00000000 0x0 0x40000000>;
>>> +	};
>>> +};
>>> +
>>> +&mdio {
>>> +	phy0: ethernet-phy@0 {
>>> +		reg = <0>;
>>> +	};
>>> +};
>>> +
>>> +&i2c0 {
>>> +	status = "okay";
>>> +};
>>> +
>>> +&i2c1 {
>>> +	status = "okay";
>>> +};
>>> +
>>> +&eth0 {
>>> +	status = "okay";
>>> +	phy-handle = <&phy0>;
>>> +};
>>> +
>>> +&usb0 {
>>> +	status = "okay";
>>> +};
>>> +
>>> +&usb1 {
>>> +	status = "okay";
>>> +};
>>> +
>>> +&spi0 {
>>> +	status = "okay";
>>> +
>>> +	spiflash0: flash@0 {
>>> +		compatible = "jedec,spi-nor";
>>> +		spi-max-frequency = <50000000>;
>>> +		spi-tx-bus-width = <1>; /* 1-single, 2-dual, 4-quad */
>>> +		spi-rx-bus-width = <1>; /* 1-single, 2-dual, 4-quad */
>>> +		reg = <0>;
>>> +
>>> +		#address-cells = <1>;
>>> +		#size-cells = <1>;
>>> +
>>> +		partition@0 {
>>> +			label = "spi_flash_part0";
>>> +			reg = <0x0 0x800000>;
>>> +		};
>>> +
>>> +		parition@1 {
>>> +			label = "spi_flash_part1";
>>> +			reg = <0x800000 0x700000>;
>>> +		};
>>> +
>>> +		parition@2 {
>>> +			label = "spi_flash_part2";
>>> +			reg = <0xF00000 0x100000>;
>>> +		};
>>> +	};
>>> +};
>>> +
>>> +&usb1 {
>>> +	compatible = "chipidea,usb2";
>> Why compatible is defined per board?
> 
> That came from the Marvell SDK. On some boards this is used as a 
> device/OTG interface. But regardless it should have one in the SoC dtsi. 

Yes, please.

> As for why they used the "chipidea,usb2" compatible I have no idea. I'll 
> remove this and add the correct compatible to the SoC.

They could reuse some other block. Pretty often for such cases there is
a dedicated compatible and fallback, e.g.:
Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml

Best regards,
Krzysztof