Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <1cda400d-7ca1-9dcc-1d33-427dfd4ec92b@linaro.org>
Date:   Tue, 11 Oct 2022 15:53:34 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.2
Subject: Re: [PATCH v1 3/5] ARM: dts: hpe: Add PLREG/SPI Support
Content-Language: en-US
To:     nick.hawkins@hpe.com, verdun@hpe.com, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, linux@armlinux.org.uk,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org
References: <20221011185525.94210-1-nick.hawkins@hpe.com>
 <20221011185525.94210-4-nick.hawkins@hpe.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20221011185525.94210-4-nick.hawkins@hpe.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 11/10/2022 14:55, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> Adding support for the Programmable Logic Register driver in the HPE GXP
> SoC. Additionally adding support for the SPI driver that has already
> been committed to linux (See: drivers/spi/spi-gxp.c).
> 
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> ---
>  arch/arm/boot/dts/hpe-bmc-dl360gen10.dts | 275 +++++++++++++++++++++++
>  arch/arm/boot/dts/hpe-gxp.dtsi           |  28 ++-
>  2 files changed, 302 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
> index 3a7382ce40ef..c97b052c4868 100644
> --- a/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
> +++ b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
> @@ -23,4 +23,279 @@
>  		device_type = "memory";
>  		reg = <0x40000000 0x20000000>;
>  	};
> +
> +	leds {
> +		compatible = "gpio-leds";
> +
> +		led-power {
> +			gpios = <&plreg 6 0>;
> +			default_state = "off";

Use generic properties for color and function. Same applies to other nodes.

> +		};
> +
> +		led-heartbeat {
> +			gpios = <&plreg 7 0>;
> +			default_state = "off";
> +		};
> +
> +		led-identify {
> +			gpios = <&plreg 56 0>;
> +			default-state = "off";
> +		};
> +
> +		led-health_red {
> +			gpios = <&plreg 57 0>;
> +			default_state = "off";
> +		};
> +
> +		led-health_amber {
> +			gpios = <&plreg 58 0>;
> +			default-state = "off";
> +		};
> +	};
> +};
> +
> +&spifi {
> +	status = "okay";

Blank line

> +	flash@0 {
> +		partitions {
> +			compatible = "fixed-partitions";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +
> +			u-boot@0 {
> +				label = "u-boot";
> +				reg = <0x0 0x60000>;
> +			};
> +
> +			u-boot-env@60000 {
> +				label = "u-boot-env";
> +				reg = <0x60000 0x20000>;
> +			};
> +
> +			kernel@80000 {
> +				label = "kernel";
> +				reg = <0x80000 0x4c0000>;
> +			};
> +
> +			rofs@540000 {
> +				label = "rofs";
> +				reg = <0x540000 0x1740000>;
> +			};
> +
> +			rwfs@1c80000 {
> +				label = "rwfs";
> +				reg = <0x1c80000 0x250000>;
> +			};
> +
> +			section@1ed0000{
> +				label = "section";
> +				reg = <0x1ed0000 0x130000>;
> +			};
> +		};
> +	};

Blank line


> +	flash@1 {
> +		partitions {
> +			compatible = "fixed-partitions";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +
> +			host-prime@0 {
> +				label = "host-prime";
> +				reg = <0x0 0x02000000>;
> +			};
> +
> +			host-second@2000000 {
> +				label = "host-second";
> +				reg = <0x02000000 0x02000000>;
> +			};
> +		};
> +	};
> +};
> +
> +&plreg {
> +	gpio-controller;
> +	#gpio-cells = <2>;
> +	gpio-line-names =
> +	"", "", "", "", "",

Messed indentation.

> +	"", "POWER", "HEARTBEAT", "FAN1_INST", "FAN2_INST",
> +	"FAN3_INST", "FAN4_INST", "FAN5_INST", "FAN6_INST", "FAN7_INST",
> +	"FAN8_INST", "FAN9_INST", "FAN10_INST", "FAN11_INST", "FAN12_INST",
> +	"FAN13_INST", "FAN14_INST", "FAN15_INST", "FAN16_INST", "FAN1_FAIL",
> +	"FAN2_FAIL", "FAN3_FAIL", "FAN4_FAIL", "FAN5_FAIL", "FAN6_FAIL",
> +	"FAN7_FAIL", "FAN8_FAIL", "FAN9_FAIL", "FAN10_FAIL", "FAN11_FAIL",
> +	"FAN12_FAIL", "FAN13_FAIL", "FAN14_FAIL", "FAN15_FAIL", "FAN16_FAIL",
> +	"", "", "", "", "",
> +	"", "", "", "", "",
> +	"", "", "", "", "",
> +	"", "IDENTIFY", "HEALTH_RED", "HEALTH_AMBER", "POWER_BUTTON",
> +	"", "SIO_POWER_GOOD", "NMI_BUTTON", "RESET_BUTTON", "SIO_S5",
> +	"SIO_ONCONTROL", "", "", "", "",
> +	"", "", "", "", "",
> +	"", "", "", "", "",
> +	"", "", "", "", "",
> +	"", "", "", "", "",
> +	"", "", "", "", "",
> +	"", "", "", "", "";
> +	fan1 {

fan-1? Difficult to suggest as you did not explain this in the binding.

> +		inst = <0x27>;
> +		fail = <0x29>;
> +		id = <0x2B>;
> +		bit = <0x01>;
> +	};
> +	fan2 {
> +		inst = <0x27>;
> +		fail = <0x29>;
> +		id = <0x2B>;
> +		bit = <0x02>;
> +	};
> +	fan3 {
> +		inst = <0x27>;
> +		fail = <0x29>;
> +		id = <0x2B>;
> +		bit = <0x04>;
> +	};
> +	fan4 {
> +		inst = <0x27>;
> +		fail = <0x29>;
> +		id = <0x2B>;
> +		bit = <0x08>;
> +	};
> +	fan5 {
> +		inst = <0x27>;
> +		fail = <0x29>;
> +		id = <0x2B>;
> +		bit = <0x10>;
> +	};
> +	fan6 {
> +		inst = <0x27>;
> +		fail = <0x29>;
> +		id = <0x2B>;
> +		bit = <0x40>;
> +	};
> +	fan7 {
> +		inst = <0x27>;
> +		fail = <0x29>;
> +		id = <0x2B>;
> +		bit = <0x40>;
> +	};
> +	fan8 {
> +		inst = <0x27>;
> +		fail = <0x29>;
> +		id = <0x2B>;
> +		bit = <0x80>;
> +	};
> +	fan9 {
> +		inst = <0x28>;
> +		fail = <0x2A>;
> +		id = <0x2C>;
> +		bit = <0x01>;
> +	};
> +	fan10 {
> +		inst = <0x28>;
> +		fail = <0x2A>;
> +		id = <0x2C>;
> +		bit = <0x02>;
> +	};
> +	fan11 {
> +		inst = <0x28>;
> +		fail = <0x2A>;
> +		id = <0x2C>;
> +		bit = <0x04>;
> +	};
> +	fan12 {
> +		inst = <0x28>;
> +		fail = <0x2A>;
> +		id = <0x2C>;
> +		bit = <0x08>;
> +	};
> +	fan13 {
> +		inst = <0x28>;
> +		fail = <0x2A>;
> +		id = <0x2C>;
> +		bit = <0x10>;
> +	};
> +	fan14 {
> +		inst = <0x28>;
> +		fail = <0x2A>;
> +		id = <0x2C>;
> +		bit = <0x20>;
> +	};
> +	fan15 {
> +		inst = <0x28>;
> +		fail = <0x2A>;
> +		id = <0x2C>;
> +		bit = <0x40>;
> +	};
> +	fan16 {
> +		inst = <0x28>;
> +		fail = <0x2A>;
> +		id = <0x2C>;
> +		bit = <0x80>;
> +	};
> +	healthled {
> +		red = <0x0D 0x20>;
> +		amber = <0x0D 0x30>;
> +		green = <0x0D 0x10>;
> +	};
> +	iopled1 {
> +		on = <0x04 0x01>;
> +	};
> +	iopled2 {
> +		on = <0x04 0x02>;
> +	};
> +	iopled3 {
> +		on = <0x04 0x04>;
> +	};
> +	iopled4 {
> +		on = <0x04 0x08>;
> +	};
> +	iopled5 {
> +		on = <0x04 0x10>;
> +	};
> +	iopled6 {
> +		on = <0x04 0x20>;
> +	};
> +	iopled7 {
> +		on = <0x04 0x40>;
> +	};
> +	iopled8 {
> +		on = <0x04 0x80>;
> +	};
> +	identifyled {
> +		on = <0x05 0xC0>;
> +		off = <0x05 0x40>;
> +	};
> +	acm {
> +		forceoff = <0x0A 0x01>;
> +		removed = <0x0A 0x02>;
> +		unlatchreq = <0x0A 0x04>;
> +	};
> +	serverid {
> +		lower = <0x01 0xFF>;
> +		upper = <0x02 0xFF>;
> +	};
> +	sideband {
> +		disabled = <0x40 0x03>;
> +		embedded = <0x40 0x02>;
> +		adaptive = <0x40 0x01>;
> +	};
> +	grp5intflag {
> +		ack = <0xB0 0xFF>;
> +		pwrbtn = <0xB0 0x01>;
> +		uidpress = <0xB0 0x02>;
> +		slpint = <0xB0 0x04>;
> +	};
> +	grp5intmask {
> +		pwrbtn = <0xB1 0x01>;
> +		slpint = <0xB1 0x40>;
> +	};
> +	grpintsmasks {
> +		grp5 = <0x88 0x10>;
> +	};
> +	grpintsflags {
> +		grp5 = <0x8C 0x10>;
> +	};
> +	pwrbtn {
> +		latch = <0x0F 0xFF 0x04>;
> +	};
>  };
> diff --git a/arch/arm/boot/dts/hpe-gxp.dtsi b/arch/arm/boot/dts/hpe-gxp.dtsi
> index cf735b3c4f35..96003667bebe 100644
> --- a/arch/arm/boot/dts/hpe-gxp.dtsi
> +++ b/arch/arm/boot/dts/hpe-gxp.dtsi
> @@ -56,9 +56,28 @@
>  			compatible = "simple-bus";
>  			#address-cells = <1>;
>  			#size-cells = <1>;
> -			ranges = <0x0 0xc0000000 0x30000000>;
> +			ranges = <0x0 0xc0000000 0x40000000>;
>  			dma-ranges;
>  
> +			spifi: spi@200 {
> +				compatible = "hpe,gxp-spifi";
> +				reg = <0x200 0x80>, <0xc000 0x100>, <0x38000000 0x8000000>;
> +				interrupts = <20>;
> +				interrupt-parent = <&vic0>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				status = "disabled";

Blank line.

> +				flash@0 {
> +					reg = <0>;
> +					compatible = "jedec,spi-nor";
> +				};
> +
> +				flash@1 {
> +					reg = <1>;
> +					compatible = "jedec,spi-nor";
> +				};
> +			};
> +
>  			vic0: interrupt-controller@eff0000 {
>  				compatible = "arm,pl192-vic";
>  				reg = <0xeff0000 0x1000>;
> @@ -122,6 +141,13 @@
>  				interrupts = <6>;
>  				interrupt-parent = <&vic0>;
>  			};
> +
> +			plreg: plreg@d1000000 {

Use same node name as in bindings example...

> +				compatible = "hpe,gxp-plreg", "simple-mfd", "syscon";
> +				reg = <0xd1000000 0xff>;
> +				interrupts = <26>;
> +				interrupt-parent = <&vic0>;
> +			};
>  		};
>  	};
>  };

Best regards,
Krzysztof