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

[Logan B <mrbojangles3@gmail.com>]

On Fri, Feb 10, 2023 at 7:32 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> 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
>
Thank you for your comments and instructions. I will use the scripts
provided in the kernel and make the changes you have indicated.

-- 
Logan