Date:   Wed, 11 May 2022 23:49:21 +0000
From:   Chris Packham <Chris.Packham@...iedtelesis.co.nz>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        "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/22 04:38, Krzysztof Kozlowski wrote:
> On 11/05/2022 01:10, Chris Packham wrote:
>> The 98DX2530 SoC is the Control and Management CPU integrated into
>> the Marvell 98DX25xx and 98DX35xx series of switch chip (internally
>> referred to as AlleyCat5 and AlleyCat5X).
>>
>> These files have been taken from the Marvell SDK and lightly cleaned
>> up with the License and copyright retained.
>>
>> Signed-off-by: Chris Packham <chris.packham@...iedtelesis.co.nz>
>> Reviewed-by: Andrew Lunn <andrew@...n.ch>
>> ---
>>
>> Notes:
>>      The Marvell SDK has a number of new compatible strings. I've brought
>>      through some of the drivers or where possible used an in-tree
>>      alternative (e.g. there is SDK code for a ac5-gpio but two instances of
>>      the existing marvell,orion-gpio seems to cover what is needed if you use
>>      an appropriate binding). I expect that there will a new series of
>>      patches when I get some different hardware (or additions to this series
>>      depending on if/when it lands).
>>      
>>      Changes in v6:
>>      - Move CPU nodes above the SoC (Krzysztof)
>>      - Minor formatting clean ups (Krzysztof)
>>      - Run through `make dtbs_check`
>>      - Move gic nodes inside SoC
>>      - Group clocks under a clock node
>>      Changes in v5:
>>      - add #{address,size}-cells property to i2c nodes
>>      - make i2c nodes disabled in the SoC and enable them in the board
>>      - add interrupt controller attributes to gpio nodes
>>      - Move fixed-clock nodes up a level and remove unnecessary @0
>>      Changes in v4:
>>      - use 'phy-handle' instead of 'phy'
>>      - move status="okay" on usb nodes to board dts
>>      - Add review from Andrew
>>      Changes in v3:
>>      - Move memory node to board
>>      - Use single digit reg value for phy address
>>      - Remove MMC node (driver needs work)
>>      - Remove syscon & simple-mfd for pinctrl
>>      Changes in v2:
>>      - Make pinctrl a child node of a syscon node
>>      - Use marvell,armada-8k-gpio instead of orion-gpio
>>      - Remove nand peripheral. The Marvell SDK does have some changes for the
>>        ac5-nand-controller but I currently lack hardware with NAND fitted so
>>        I can't test it right now. I've therefore chosen to omit the node and
>>        not attempted to bring in the driver or binding.
>>      - Remove pcie peripheral. Again there are changes in the SDK and I have
>>        no way of testing them.
>>      - Remove prestera node.
>>      - Remove "marvell,ac5-ehci" compatible from USB node as
>>        "marvell,orion-ehci" is sufficient
>>      - Remove watchdog node. There is a buggy driver for the ac5 watchdog in
>>        the SDK but it needs some work so I've dropped the node for now.
>>
>>   arch/arm64/boot/dts/marvell/Makefile          |   1 +
>>   .../boot/dts/marvell/armada-98dx2530.dtsi     | 313 ++++++++++++++++++
>>   arch/arm64/boot/dts/marvell/rd-ac5x.dts       |  90 +++++
>>   3 files changed, 404 insertions(+)
>>   create mode 100644 arch/arm64/boot/dts/marvell/armada-98dx2530.dtsi
>>   create mode 100644 arch/arm64/boot/dts/marvell/rd-ac5x.dts
>>
>> diff --git a/arch/arm64/boot/dts/marvell/Makefile b/arch/arm64/boot/dts/marvell/Makefile
>> index 1c794cdcb8e6..3905dee558b4 100644
>> --- a/arch/arm64/boot/dts/marvell/Makefile
>> +++ b/arch/arm64/boot/dts/marvell/Makefile
>> @@ -24,3 +24,4 @@ dtb-$(CONFIG_ARCH_MVEBU) += cn9132-db.dtb
>>   dtb-$(CONFIG_ARCH_MVEBU) += cn9132-db-B.dtb
>>   dtb-$(CONFIG_ARCH_MVEBU) += cn9130-crb-A.dtb
>>   dtb-$(CONFIG_ARCH_MVEBU) += cn9130-crb-B.dtb
>> +dtb-$(CONFIG_ARCH_MVEBU) += rd-ac5x.dtb
>> diff --git a/arch/arm64/boot/dts/marvell/armada-98dx2530.dtsi b/arch/arm64/boot/dts/marvell/armada-98dx2530.dtsi
>> new file mode 100644
>> index 000000000000..724e722b4265
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/marvell/armada-98dx2530.dtsi
>> @@ -0,0 +1,313 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Device Tree For AC5.
>> + *
>> + * Copyright (C) 2021 Marvell
>> + *
>> + */
>> +
>> +/dts-v1/;
>> +
>> +#include <dt-bindings/gpio/gpio.h>
>> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>> +
>> +/ {
>> +	model = "Marvell AC5 SoC";
>> +	compatible = "marvell,ac5";
>> +	interrupt-parent = <&gic>;
>> +	#address-cells = <2>;
>> +	#size-cells = <2>;
>> +
>> +	aliases {
>> +		serial0 = &uart0;
>> +		spiflash0 = &spiflash0;
> These should be in board DTS, not SoC.
>
> https://lore.kernel.org/linux-rockchip/CAK8P3a25iYksubCnQb1-e5yj=crEsK37RB9Hn4ZGZMwcVVrG7g@mail.gmail.com/

Thanks for the reference. Will move.

>
>> +		gpio0 = &gpio0;
>> +		gpio1 = &gpio1;
>> +		ethernet0 = &eth0;
>> +		ethernet1 = &eth1;
> (...)
>
>> +
>> +	clocks {
>> +		core_clock: core-clock {
>> +			compatible = "fixed-clock";
>> +			#clock-cells = <0>;
>> +			clock-frequency = <400000000>;
>> +		};
>> +
>> +		axi_clock: axi-clock {
>> +			compatible = "fixed-clock";
>> +			#clock-cells = <0>;
>> +			clock-frequency = <325000000>;
>> +		};
>> +
>> +		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.

>> +	};
>> +};
>> 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";

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

>> +	phys = <&usb1phy>;
>> +	phy-names = "usb-phy";
>> +	dr_mode = "peripheral";
>> +};
>> +
>
> Best regards,
> Krzysztof

Powered by blists - more mailing lists