Re: [PATCH 2/2] arm64: dts: add description for solidrun cn9130 som and clearfog boards

[Josua Mayer <josua@solid-run.com>]

Am 21.03.24 um 22:59 schrieb Andrew Lunn:
> On Thu, Mar 21, 2024 at 10:47:12PM +0100, Josua Mayer wrote:
>> Add description for the SolidRun CN9130 SoM, and Clearfog Base / Pro
>> reference boards.
>>
>> The SoM has been designed as a pin-compatible replacement for the older
>> Armada 388 based SoM. Therefore it supports the same boards and a
>> similar feature set.
>>
>> Most notable upgrades:
>> - 4x Cortex-A72
>> - 10Gbps SFP
>> - Both eMMC and SD supported at the same time
>>
>> The developer first supporting this product at SolidRun decided to use
>> different filenames for the DTBs: Armada 388 uses the full
>> "clearfog" string while cn9130 uses the abbreviation "cf".
>> This name is already hard-coded in pre-installed vendor u-boot and can
>> not be changed easily.
>>
>> NOTICE IN CASE ANYBODY WANTS TO SELF-UPGRADE:
>> CN9130 SoM has a different footprint from Armada 388 SoM.
>> Components on the carrier board below the SoM may collide causing
>> damage, such as on Clearfog Base.
>>
>> Signed-off-by: Josua Mayer <josua@solid-run.com>
>> ---
>>  arch/arm64/boot/dts/marvell/Makefile           |   2 +
>>  arch/arm64/boot/dts/marvell/cn9130-cf-base.dts | 138 ++++++++++++++
>>  arch/arm64/boot/dts/marvell/cn9130-cf-pro.dts  | 249 +++++++++++++++++++++++++
>>  arch/arm64/boot/dts/marvell/cn9130-cf.dtsi     | 198 ++++++++++++++++++++
>>  arch/arm64/boot/dts/marvell/cn9130-sr-som.dtsi | 160 ++++++++++++++++
>>  5 files changed, 747 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/marvell/Makefile b/arch/arm64/boot/dts/marvell/Makefile
>> index 99b8cb3c49e1..019f2251d696 100644
>> --- a/arch/arm64/boot/dts/marvell/Makefile
>> +++ b/arch/arm64/boot/dts/marvell/Makefile
>> @@ -28,3 +28,5 @@ dtb-$(CONFIG_ARCH_MVEBU) += cn9130-crb-A.dtb
>>  dtb-$(CONFIG_ARCH_MVEBU) += cn9130-crb-B.dtb
>>  dtb-$(CONFIG_ARCH_MVEBU) += ac5x-rd-carrier-cn9131.dtb
>>  dtb-$(CONFIG_ARCH_MVEBU) += ac5-98dx35xx-rd.dtb
>> +dtb-$(CONFIG_ARCH_MVEBU) += cn9130-cf-base.dtb
>> +dtb-$(CONFIG_ARCH_MVEBU) += cn9130-cf-pro.dtb
>> diff --git a/arch/arm64/boot/dts/marvell/cn9130-cf-base.dts b/arch/arm64/boot/dts/marvell/cn9130-cf-base.dts
>> new file mode 100644
>> index 000000000000..b0067940d5e4
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/marvell/cn9130-cf-base.dts
>> @@ -0,0 +1,138 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright (C) 2024 Josua Mayer <josua@solid-run.com>
>> + *
>> + * DTS for SolidRun CN9130 Clearfog Base.
>> + *
>> + */
>> +
>> +/dts-v1/;
>> +
>> +#include <dt-bindings/input/input.h>
>> +
>> +#include "cn9130.dtsi"
>> +#include "cn9130-sr-som.dtsi"
>> +#include "cn9130-cf.dtsi"
>> +
>> +/ {
>> +	model = "SolidRun CN9130 Clearfog Base";
>> +	compatible = "solidrun,clearfog-base-a1", "solidrun,clearfog-a1",
>> +		     "solidrun,cn9130-sr-som","marvell,cn9130",
>> +		     "marvell,armada-ap807-quad", "marvell,armada-ap807";
>> +
>> +	gpio-keys {
>> +		compatible = "gpio-keys";
>> +		pinctrl-0 = <&rear_button_pins>;
>> +		pinctrl-names = "default";
>> +
>> +		button-0 {
>> +			/* The rear SW3 button */
>> +			label = "Rear Button";
>> +			gpios = <&cp0_gpio1 31 GPIO_ACTIVE_LOW>;
>> +			linux,can-disable;
>> +			linux,code = <BTN_0>;
>> +		};
>> +	};
>> +
>> +	rfkill-m2-gnss {
>> +		compatible = "rfkill-gpio";
>> +		label = "m.2 GNSS";
>> +		radio-type = "gps";
>> +		/* rfkill-gpio inverts internally */
>> +		shutdown-gpios = <&expander0 9 GPIO_ACTIVE_HIGH>;
>> +	};
>> +
>> +	/* M.2 is B-keyed, so w-disable is for WWAN */
>> +	rfkill-m2-wwan {
>> +		compatible = "rfkill-gpio";
>> +		label = "m.2 WWAN";
>> +		radio-type = "wwan";
>> +		/* rfkill-gpio inverts internally */
>> +		shutdown-gpios = <&expander0 8 GPIO_ACTIVE_HIGH>;
>> +	};
>> +};
>> +
>> +/* SRDS #3 - SGMII 1GE */
>> +&cp0_eth1 {
>> +	phy = <&phy1>;
>> +	phys = <&cp0_comphy3 1>;
>> +	phy-mode = "sgmii";
>> +	status = "okay";
>> +};
>> +
>> +&cp0_eth2_phy {
>> +	/*
>> +	 * Configure LEDs:
>> +	 * - LED[0]: link/activity: On/blink (green)
>> +	 * - LED[1]: link is 100/1000Mbps: On (yellow)
>> +	 * - LED[2]: high impedance (floating)
>> +	 */
>> +	marvell,reg-init = <3 16 0xf000 0x0a61>;
> Sorry, but no. List the LEDs in the PHY node, and they can then be
> controlled via /sys/class/leds.
May I ask more precisely the motivation?
Does this replace the phy's builtin automatic led control?
> arch/arm/boot/dts/marvell/armada-370-rd.dts is an example.

I will investigate it.

My main motivation for tweaking the led controls was to make them all consistent across the two boards:
- LEDs under control of PHYs on cpu mdio bus
- LEDs under control of ethernet switch on mdio bus
- LEDs under control of ethernet phy on external mdio bus behind ethernet switch

It looks as if the marvell phy driver supports led subnodes,
The switch driver does not.
Finally one phy can only be written to but not read,
the cpu can never know its link state.

So I prefer (for the Clearfog Pro) board to explicitly use the phys
autonomous management of LEDs.
Is that still possible if I added led subnodes?