Hi, Stefan,
Thank you for your review! I will put all required changes into second patch version.
Regarding the symbolic names for the pin controller functions and lack of documentation. The problem is that same function number does not have the same meaning for different pins. So if I want to put symbolic names instead of numbers, I have to add large structures defining symbolic names for each function on every pin as a platform data. I think in this case I will loose the driver code compactness provided by the FDT usage. I can create a documentation file with all pin function values taken from SoC HW manual and keep the numeric function IDs for the DTS usage. Will it be good enough?
Best Regards Konstantin ________________________________________ From: Stefan Roese sr@denx.de Sent: Thursday, November 24, 2016 11:13 To: Kostya Porotchkin; u-boot@lists.denx.de Cc: Nadav Haklai; Neta Zur Hershkovits; Omri Itach; Igal Liberman; Haim Boot; Hanna Hawa Subject: Re: [PATCH 4/6] arm64: mvebu: Add pin control nodes to A8K family DTS files
Hi Kosta,
On 20.11.2016 16:38, kostap@marvell.com wrote:
From: Konstantin Porotchkin kostap@marvell.com
Add pin control nodes to APN806, CP-master, CP-slave and Armada-7040 and Armada-8040 boards DTS files
Change-Id: I51522f33f23e3f9e94c6f5a5f9af19f5dc86e6b7 Signed-off-by: Konstantin Porotchkin kostap@marvell.com Cc: Stefan Roese sr@denx.de Cc: Nadav Haklai nadavh@marvell.com Cc: Neta Zur Hershkovits neta@marvell.com Cc: Omri Itach omrii@marvell.com Cc: Igal Liberman igall@marvell.com Cc: Haim Boot hayim@marvell.com Cc: Hanna Hawa hannah@marvell.com
arch/arm/dts/armada-7040-db.dts | 38 +++++++++++++++++++++++ arch/arm/dts/armada-8040-db.dts | 57 +++++++++++++++++++++++++++++++++++ arch/arm/dts/armada-ap806.dtsi | 18 +++++++++++ arch/arm/dts/armada-cp110-master.dtsi | 32 ++++++++++++++++++++ arch/arm/dts/armada-cp110-slave.dtsi | 19 ++++++++++++ 5 files changed, 164 insertions(+)
diff --git a/arch/arm/dts/armada-7040-db.dts b/arch/arm/dts/armada-7040-db.dts index b8fe5a9..1bfb5a9 100644 --- a/arch/arm/dts/armada-7040-db.dts +++ b/arch/arm/dts/armada-7040-db.dts @@ -67,6 +67,8 @@ };
&i2c0 {
pinctrl-names = "default";pinctrl-0 = <&cpm_i2c0_pins>; status = "okay"; clock-frequency = <100000>;}; @@ -98,6 +100,17 @@ }; };
+&ap_pinctl {
/* MPP Bus:SDIO [0-10]UART0 [11,19]*/
Please use the common multiline comment instead:
/* * MPP Bus: * SDIO [0-10] * UART0 [11,19] */
/* 0 1 2 3 4 5 6 7 8 9 */pin-func = < 1 1 1 1 1 1 1 1 1 11 3 0 0 0 0 0 0 0 3>;
Is there any chance to not use those numeric values but some macros instead to make it clearer, which function is selected?
status = "okay";+};
&uart0 { status = "okay"; }; @@ -112,8 +125,33 @@ clock-frequency = <100000>; };
+&cpm_pinctl {
/* MPP Bus:TDM [0-11]SPI [13-16]SATA1 [28]UART0 [29-30]SMI [32,34]XSMI [35-36]I2C [37-38]RGMII1[44-55]SD [56-62]*/
Again, comment style please.
/* 0 1 2 3 4 5 6 7 8 9 */pin-func = < 4 4 4 4 4 4 4 4 4 44 4 0 3 3 3 3 0 0 00 0 0 0 0 0 0 0 9 0xA0xA 0 7 0 7 7 7 2 2 00 0 0 0 1 1 1 1 1 11 1 1 1 1 1 0xE 0xE 0xE 0xE0xE 0xE 0xE>;
Lower case hex values please (globally).
status = "okay";+};
&cpm_spi1 { status = "okay";
pinctrl-names = "default";pinctrl-0 = <&cpm_spi0_pins>; spi-flash@0 { #address-cells = <0x1>;diff --git a/arch/arm/dts/armada-8040-db.dts b/arch/arm/dts/armada-8040-db.dts index 86666a1..30fd364 100644 --- a/arch/arm/dts/armada-8040-db.dts +++ b/arch/arm/dts/armada-8040-db.dts @@ -71,6 +71,42 @@ status = "okay"; };
+&ap_pinctl {
/* MPP Bus:SPI0 [0-3]I2C0 [4-5]UART0 [11,19]*//* 0 1 2 3 4 5 6 7 8 9 */pin-func = < 3 3 3 3 3 3 0 0 0 00 3 0 0 0 0 0 0 0 3>;+};
+&cpm_pinctl {
/* MPP Bus:[0-31] = 0xff: Keep default CP0_shared_pins:[11] CLKOUT_MPP_11 (out)[23] LINK_RD_IN_CP2CP (in)[25] CLKOUT_MPP_25 (out)[29] AVS_FB_IN_CP2CP (in)[32,34] SMI[31] GPIO: push button/Wake[35-36] GPIO[37-38] I2C[40-41] SATA[0/1]_PRESENT_ACTIVEn[42-43] XSMI[44-55] RGMII1[56-62] SD*//* 0 1 2 3 4 5 6 7 8 9 */pin-func = < 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff0xff 0 7 0 7 0 0 2 2 00 0 8 8 1 1 1 1 1 11 1 1 1 1 1 0xE 0xE 0xE 0xE0xE 0xE 0xE>;+};
/* CON5 on CP0 expansion */ &cpm_pcie2 { @@ -78,6 +114,8 @@ };
&cpm_i2c0 {
pinctrl-names = "default";pinctrl-0 = <&cpm_i2c0_pins>; status = "okay"; clock-frequency = <100000>;}; @@ -97,12 +135,31 @@ status = "okay"; };
+&cps_pinctl {
/* MPP Bus:[0-11] RGMII0[13-16] SPI1[27,31] GE_MDIO/MDC[32-62] = 0xff: Keep default CP1_shared_pins:*//* 0 1 2 3 4 5 6 7 8 9 */pin-func = < 0x3 0x3 0x3 0x3 0x3 0x3 0x3 0x3 0x3 0x30x3 0x3 0xff 0x3 0x3 0x3 0x3 0xff 0xff 0xff0xff 0xff 0xff 0xff 0xff 0xff 0xff 0x8 0xff 0xff0xff 0x8 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff0xff 0xff 0xff>;+};
/* CON5 on CP1 expansion */ &cps_pcie2 { status = "okay"; };
&cps_spi1 {
pinctrl-names = "default";pinctrl-0 = <&cps_spi1_pins>; status = "okay"; spi-flash@0 {diff --git a/arch/arm/dts/armada-ap806.dtsi b/arch/arm/dts/armada-ap806.dtsi index d315b29..efb383b 100644 --- a/arch/arm/dts/armada-ap806.dtsi +++ b/arch/arm/dts/armada-ap806.dtsi @@ -140,6 +140,24 @@ marvell,spi-base = <128>, <136>, <144>, <152>; };
ap_pinctl: ap-pinctl@6F4000 {compatible = "marvell,armada-ap806-pinctrl";bank-name ="apn-806";reg = <0x6F4000 0x10>;pin-count = <20>;max-func = <3>;ap_i2c0_pins: i2c-pins-0 {marvell,pins = < 4 5 >;marvell,function = <3>;
So what are these marvell,pins/functions? They are not listed in the DT bindings documentation.
};ap_emmc_pins: emmc-pins-0 {marvell,pins = < 0 1 2 3 4 5 6 78 9 10 >;marvell,function = <1>;};};xor@400000 { compatible = "marvell,mv-xor-v2"; reg = <0x400000 0x1000>,diff --git a/arch/arm/dts/armada-cp110-master.dtsi b/arch/arm/dts/armada-cp110-master.dtsi index 422d754..d637867 100644 --- a/arch/arm/dts/armada-cp110-master.dtsi +++ b/arch/arm/dts/armada-cp110-master.dtsi @@ -81,6 +81,38 @@ "cpm-usb3dev", "cpm-eip150", "cpm-eip197"; };
cpm_pinctl: cpm-pinctl@440000 {compatible = "marvell,mvebu-pinctrl","marvell,a70x0-pinctrl","marvell,a80x0-cp0-pinctrl";bank-name ="cp0-110";reg = <0x440000 0x20>;pin-count = <63>;max-func = <0xf>;cpm_i2c0_pins: cpm-i2c-pins-0 {marvell,pins = < 37 38 >;marvell,function = <2>;};cpm_ge2_rgmii_pins: cpm-ge-rgmii-pins-0 {marvell,pins = < 44 45 46 47 48 49 50 5152 53 54 55 >;marvell,function = <1>;};pca0_pins: cpm-pca0_pins {marvell,pins = <62>;marvell,function = <0>;};cpm_sdhci_pins: cpm-sdhi-pins-0 {marvell,pins = < 56 57 58 59 60 61 >;marvell,function = <14>;};cpm_spi0_pins: cpm-spi-pins-0 {marvell,pins = < 13 14 15 16 >;marvell,function = <3>;};};cpm_sata0: sata@540000 { compatible = "marvell,armada-8k-ahci"; reg = <0x540000 0x30000>;diff --git a/arch/arm/dts/armada-cp110-slave.dtsi b/arch/arm/dts/armada-cp110-slave.dtsi index a7f77b9..92ef55c 100644 --- a/arch/arm/dts/armada-cp110-slave.dtsi +++ b/arch/arm/dts/armada-cp110-slave.dtsi @@ -81,6 +81,25 @@ "cps-usb3dev", "cps-eip150", "cps-eip197"; };
cps_pinctl: cps-pinctl@440000 {compatible = "marvell,mvebu-pinctrl","marvell,a80x0-cp1-pinctrl";bank-name ="cp1-110";reg = <0x440000 0x20>;pin-count = <63>;max-func = <0xf>;cps_ge1_rgmii_pins: cps-ge-rgmii-pins-0 {marvell,pins = < 0 1 2 3 4 5 6 78 9 10 11 >;marvell,function = <3>;};cps_spi1_pins: cps-spi-pins-1 {marvell,pins = < 13 14 15 16 >;marvell,function = <3>;};};cps_sata0: sata@540000 { compatible = "marvell,armada-8k-ahci"; reg = <0x540000 0x30000>;
Thanks, Stefan