Re: [PATCH] Add support for SolidRun Clearfog CN9130 base,pro.

[Krzysztof Kozlowski <krzk@kernel.org>]

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC.  It might happen, that command when run on an older
kernel, gives you outdated entries.  Therefore please be sure you base
your patches on recent Linux kernel.

There is no one to apply your patch as you missed several people.

On 13/01/2023 20:28, Logan Blyth wrote:

Thank you for your patch. There is something to discuss/improve.

1. Use subject prefixes matching the subsystem (which you can get for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching).

2. Subject: no full stop.

> From: Logan Blyth <mrbojangles3@gmail.com>
> 

3. Missing commit msg. Please describe here hardware.


> Signed-off-by: Logan Blyth <mrbojangles3@gmail.com>
> ---
>  arch/arm64/boot/dts/marvell/Makefile          |   2 +
>  .../arm64/boot/dts/marvell/cn9130-cf-base.dts | 367 +++++++++++++++
>  arch/arm64/boot/dts/marvell/cn9130-cf-pro.dts | 428 ++++++++++++++++++

4. As I wrote on IRC, missing bindings patch (first in the series).

>  3 files changed, 797 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/marvell/cn9130-cf-base.dts
>  create mode 100644 arch/arm64/boot/dts/marvell/cn9130-cf-pro.dts
> 
> diff --git a/arch/arm64/boot/dts/marvell/Makefile b/arch/arm64/boot/dts/marvell/Makefile
> index 058237681fe5..6b3b4c4856c1 100644
> --- a/arch/arm64/boot/dts/marvell/Makefile
> +++ b/arch/arm64/boot/dts/marvell/Makefile
> @@ -25,4 +25,6 @@ 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) += cn9130-cf-pro.dtb
> +dtb-$(CONFIG_ARCH_MVEBU) += cn9130-cf-base.dtb
>  dtb-$(CONFIG_ARCH_MVEBU) += ac5-98dx35xx-rd.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..f258a539e378
> --- /dev/null
> +++ b/arch/arm64/boot/dts/marvell/cn9130-cf-base.dts
> @@ -0,0 +1,367 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright SolidRun Ltd.
> + *
> + * Device tree for the	CN9130 based SOM.

5. Drop weird space/indent in the middle.

> + */
> +
> +#include "cn9130.dtsi"
> +
> +#include <dt-bindings/gpio/gpio.h>
> +
> +/ {
> +	model = "SolidRun CN9130 based SOM Clearfog Base";

6. Missing compatible.

> +
> +	chosen {
> +		stdout-path = "serial0:115200n8";
> +	};
> +
> +	aliases {
> +		gpio1 = &cp0_gpio1;
> +		gpio2 = &cp0_gpio2;
> +		i2c0 = &cp0_i2c0;
> +		ethernet0 = &cp0_eth0;
> +		ethernet1 = &cp0_eth1;
> +		ethernet2 = &cp0_eth2;
> +		spi1 = &cp0_spi0;
> +		spi2 = &cp0_spi1;
> +	};
> +
> +	memory@00000000 {
> +		device_type = "memory";
> +		reg = <0x0 0x0 0x0 0x80000000>;
> +	};

Missing blank line.

> +	v_3_3: regulator-3-3v {
> +		compatible = "regulator-fixed";
> +		regulator-name = "v_3_3";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		regulator-always-on;
> +		status = "okay";

Drop, it's by default okay.

> +	};

Missing blank line.

> +	ap0_reg_sd_vccq: ap0_sd_vccq@0 {

Do not use undercores in node names.

Node names should contain generic prefix or suffix. In your case -
"regulator-" prefix as your node above.


> +		compatible = "regulator-gpio";
> +		regulator-name = "ap0_sd_vccq";
> +		regulator-min-microvolt = <1800000>;
> +		regulator-max-microvolt = <1800000>;
> +		states = <1800000 0x1 3300000 0x0>;
> +	};
> +
> +	cp0_reg_usb3_vbus0: cp0_usb3_vbus@0 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "cp0-xhci0-vbus";
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +		enable-active-high;

That's not correct property if you do not have GPIO... where is GPIO?

> +	};
> +
> +	cp0_usb3_0_phy0: cp0_usb3_phy@0 {

Multiple issues here...

Node names should be generic, so "phy".
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

@0 points you have reg=0, where is it? Why 0?

All problems above are also in other places.


> +		compatible = "usb-nop-xceiv";
> +		vcc-supply = <&cp0_reg_usb3_vbus0>;
> +	};
> +
> +	cp0_reg_usb3_vbus1: cp0_usb3_vbus@1 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "cp0-xhci1-vbus";
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +		enable-active-high;
> +	};
> +
> +	cp0_usb3_0_phy1: cp0_usb3_phy@1 {
> +		compatible = "usb-nop-xceiv";
> +		vcc-supply = <&cp0_reg_usb3_vbus1>;
> +	};
> +
> +	cp0_reg_sd_vccq: cp0_sd_vccq@0 {
> +		compatible = "regulator-gpio";
> +		regulator-name = "cp0_sd_vccq";
> +		regulator-min-microvolt = <1800000>;
> +		regulator-max-microvolt = <3300000>;
> +		states = <1800000 0x1
> +			3300000 0x0>;
> +	};
> +
> +	cp0_reg_sd_vcc: cp0_sd_vcc@0 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "cp0_sd_vcc";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		enable-active-high;
> +		regulator-always-on;
> +	};
> +
> +	cp0_sfp_eth0: sfp-eth@0 {
> +		compatible = "sff,sfp";
> +		i2c-bus = <&cp0_i2c1>;
> +		los-gpio = <&expander0 12 GPIO_ACTIVE_HIGH>;
> +		mod-def0-gpio = <&expander0 15 GPIO_ACTIVE_LOW>;
> +		tx-disable-gpio = <&expander0 14 GPIO_ACTIVE_HIGH>;
> +		tx-fault-gpio = <&expander0 13 GPIO_ACTIVE_HIGH>;
> +		maximum-power-milliwatt = <2000>;

Be sure you run `make dtbs_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).


> +	};
> +};
> +
> +&uart0 {

Overrides are usually ordered by name. Isn't the case for Marvell?

> +	status = "okay";
> +};
> +
> +// on-board eMMC

Comment style /* */

> +&ap_sdhci0 {
> +	pinctrl-names = "default";
> +	bus-width = <8>;
> +	vqmmc-supply = <&ap0_reg_sd_vccq>;
> +	status = "okay";
> +};
> +
> +&cp0_crypto {
> +	status = "disabled";
> +};
> +
> +&cp0_ethernet {
> +	status = "okay";
> +};
> +
> +&cp0_gpio1 {
> +	status = "okay";
> +};
> +
> +&cp0_gpio2 {
> +	status = "okay";
> +};
> +
> +// EEPROM
> +&cp0_i2c0 {
> +	status = "okay";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&cp0_i2c0_pins>;
> +	clock-frequency = <100000>;
> +
> +	/*
> +	 * PCA9655 GPIO expander, up to 1MHz clock.
> +	 *	0-CON3 CLKREQ#
> +	 *	1-CON3 PERST#
> +	 *	2-CON2 PERST#
> +	 *	3-CON3 W_DISABLE
> +	 *	4-CON2 CLKREQ#
> +	 *	5-USB3 overcurrent
> +	 *	6-USB3 power
> +	 *	7-CON2 W_DISABLE
> +	 *	8-JP4 P1
> +	 *	9-JP4 P4
> +	 * 10-JP4 P5
> +	 * 11-m.2 DEVSLP
> +	 * 12-SFP_LOS
> +	 * 13-SFP_TX_FAULT
> +	 * 14-SFP_TX_DISABLE
> +	 * 15-SFP_MOD_DEF0
> +	 */
> +	expander0: gpio-expander@20 {
> +		/*
> +		 * This is how it should be:
> +		 * compatible = "onnn,pca9655", "nxp,pca9555";
> +		 * but you can't do this because of the way I2C works.

??? Why?

> +		 */
> +		compatible = "nxp,pca9555";
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		reg = <0x20>;

reg is usually second property, after compatible

> +
> +		pcie1_0_clkreq {

No underscores in node names. Use hyphens/dashes.

> +			gpio-hog;
> +			gpios = <0 GPIO_ACTIVE_LOW>;
> +			input;
> +			line-name = "pcie1.0-clkreq";
> +		};
> +		pcie1_0_w_disable {
> +			gpio-hog;
> +			gpios = <3 GPIO_ACTIVE_LOW>;
> +			output-low;
> +			line-name = "pcie1.0-w-disable";
> +		};
> +		usb3_ilimit {
> +			gpio-hog;
> +			gpios = <5 GPIO_ACTIVE_LOW>;
> +			input;
> +			line-name = "usb3-current-limit";
> +		};
> +		usb3_power {
> +			gpio-hog;
> +			gpios = <6 GPIO_ACTIVE_HIGH>;
> +			output-high;
> +			line-name = "usb3-power";
> +		};
> +		m2_devslp {
> +			gpio-hog;
> +			gpios = <11 GPIO_ACTIVE_HIGH>;
> +			output-low;
> +			line-name = "m.2 devslp";
> +		};
> +	};
> +
> +	// The MCP3021 supports standard and fast modes
> +	mikrobus_adc: mcp3021@4c {

Node names should be generic.
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> +		compatible = "microchip,mcp3021";
> +		reg = <0x4c>;
> +	};
> +
> +	// EEPROM on the SOM
> +	eeprom@53 {
> +		compatible = "atmel,24c02";
> +		reg = <0x53>;
> +		pagesize = <16>;
> +	};
> +};
> +
> +// I2C Master
> +&cp0_i2c1 {
> +	status = "okay";
> +	clock-frequency = <100000>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&cp0_i2c1_pins>;
> +};
> +
> +&cp0_gpio1 {
> +	// Release switch reset
> +	phy_reset {

No underscores in node names

All the comments apply everywhere else. I'll stop review at this place.

Best regards,
Krzysztof