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 <s...@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, kos...@marvell.com wrote:
> From: Konstantin Porotchkin <kos...@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 <kos...@marvell.com>
> Cc: Stefan Roese <s...@denx.de>
> Cc: Nadav Haklai <nad...@marvell.com>
> Cc: Neta Zur Hershkovits <n...@marvell.com>
> Cc: Omri Itach <om...@marvell.com>
> Cc: Igal Liberman <ig...@marvell.com>
> Cc: Haim Boot <ha...@marvell.com>
> Cc: Hanna Hawa <han...@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 1
> + 1 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 4
> + 4 4 0 3 3 3 3 0 0 0
> + 0 0 0 0 0 0 0 0 9 0xA
> + 0xA 0 7 0 7 7 7 2 2 0
> + 0 0 0 0 1 1 1 1 1 1
> + 1 1 1 1 1 1 0xE 0xE 0xE 0xE
> + 0xE 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 0
> + 0 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 0xff
> + 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
> + 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
> + 0xff 0 7 0 7 0 0 2 2 0
> + 0 0 8 8 1 1 1 1 1 1
> + 1 1 1 1 1 1 0xE 0xE 0xE 0xE
> + 0xE 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 0x3
> + 0x3 0x3 0xff 0x3 0x3 0x3 0x3 0xff 0xff 0xff
> + 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0x8 0xff 0xff
> + 0xff 0x8 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
> + 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
> + 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
> + 0xff 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 7
> + 8 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 51
> + 52 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 7
> + 8 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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
