Re: [PATCH 1/2] arm64: dts: rockchip: Add core dts for RK3308 SOC

Re: [PATCH 1/2] arm64: dts: rockchip: Add core dts for RK3308 SOC

[Heiko Stuebner]

Hi Andy,

Am Donnerstag, 17. Oktober 2019, 05:04:49 CEST schrieb Andy Yan:

> +	psci {
> +		compatible = "arm,psci-1.0";
> +		method = "smc";
> +	};

Please also provide a ATF implementation for the rk3308 :-)
[Not a requirement for getting this merged, but it would be
really cool to have sources for the full stack]

> +
> +	ramoops_mem: ramoops_mem {
> +		reg = <0x0 0x110000 0x0 0xf0000>;
> +		reg-names = "ramoops_mem";
> +	};
> +
> +	ramoops: ramoops {
> +		compatible = "ramoops";
> +		record-size = <0x0 0x30000>;
> +		console-size = <0x0 0xc0000>;
> +		ftrace-size = <0x0 0x00000>;
> +		pmsg-size = <0x0 0x00000>;
> +		memory-region = <&ramoops_mem>;
> +	};

I think ramoops are more a per-board thing, like for the evb.
As they'll require cooperation with bootloaders to not mangle that
memory area. For this please also coordinate with Kever because
I somehow remember we have u-boot sometimes at 0x100000.


> +	grf: grf@ff000000 {
> +		compatible = "rockchip,rk3308-grf", "syscon", "simple-mfd";

Please add a patch adding the rockchip,rk3308-grf compatible to
Documentation/devicetree/bindings/soc/rockchip/grf.txt

> +		reg = <0x0 0xff000000 0x0 0x10000>;
> +
> +		reboot-mode {
> +			compatible = "syscon-reboot-mode";
> +			offset = <0x500>;
> +			mode-bootloader = <BOOT_BL_DOWNLOAD>;
> +			mode-loader = <BOOT_BL_DOWNLOAD>;
> +			mode-normal = <BOOT_NORMAL>;
> +			mode-recovery = <BOOT_RECOVERY>;
> +			mode-fastboot = <BOOT_FASTBOOT>;
> +		};
> +	};
> +
> +	detect_grf: syscon@ff00b000 {
> +		compatible = "syscon", "simple-mfd";

	compatible = "rockchip,rk3308-detect-grf", "syscon"
+ add the rk3308-detect-grf to the binding

> +		reg = <0x0 0xff00b000 0x0 0x1000>;
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +	};
> +
> +	core_grf: syscon@ff00c000 {
> +		compatible = "syscon", "simple-mfd";

same as detect_grf

> +		reg = <0x0 0xff00c000 0x0 0x1000>;
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +
> +	};
> +
> +	i2c0: i2c@ff040000 {
> +		compatible = "rockchip,rk3399-i2c";

	compatible = "rockchip,rk3308-i2c", "rockchip,rk3399-i2c";
Same for all i2c controllers.

> +		reg = <0x0 0xff040000 0x0 0x1000>;
> +		clocks = <&cru SCLK_I2C0>, <&cru PCLK_I2C0>;
> +		clock-names = "i2c", "pclk";
> +		interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&i2c0_xfer>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		status = "disabled";
> +	};


> +	spi0: spi@ff120000 {
> +		compatible = "rockchip,rk3308-spi", "rockchip,rk3066-spi";
> +		reg = <0x0 0xff120000 0x0 0x1000>;
> +		interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		clocks = <&cru SCLK_SPI0>, <&cru PCLK_SPI0>;
> +		clock-names = "spiclk", "apb_pclk";
> +		dmas = <&dmac0 0>, <&dmac0 1>;
> +		dma-names = "tx", "rx";
> +		pinctrl-names = "default", "high_speed";

there is no high_speed pinctrl defined for the Rockchip spi driver
in mainline, so this part should go away in a first step.
Same for the other spi controllers.

> +		pinctrl-0 = <&spi0_clk &spi0_csn0 &spi0_miso &spi0_mosi>;
> +		pinctrl-1 = <&spi0_clk_hs &spi0_csn0 &spi0_miso_hs &spi0_mosi_hs>;
> +		status = "disabled";
> +	};


> +	rktimer: rktimer@ff1a0000 {
> +		compatible = "rockchip,rk3288-timer";

	compatible = "rockchip,rk3308-timer", "rockchip,rk3288-timer";

> +		reg = <0x0 0xff1a0000 0x0 0x20>;
> +		interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&cru PCLK_TIMER>, <&cru SCLK_TIMER0>;
> +		clock-names = "pclk", "timer";
> +	};



> +	amba {
> +		compatible = "arm,amba-bus";

compatible = "simple-bus";

> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		dmac0: dma-controller@ff2c0000 {
> +			compatible = "arm,pl330", "arm,primecell";
> +			reg = <0x0 0xff2c0000 0x0 0x4000>;
> +			interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
> +			#dma-cells = <1>;
> +			clocks = <&cru ACLK_DMAC0>;
> +			clock-names = "apb_pclk";
> +			peripherals-req-type-burst;

peripherals-req-type-burst is undocumented so likely some change to the
dma driver not yet upstream?

> +		};
> +
> +		dmac1: dma-controller@ff2d0000 {
> +			compatible = "arm,pl330", "arm,primecell";
> +			reg = <0x0 0xff2d0000 0x0 0x4000>;
> +			interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
> +			#dma-cells = <1>;
> +			clocks = <&cru ACLK_DMAC1>;
> +			clock-names = "apb_pclk";
> +			peripherals-req-type-burst;
> +		};
> +	};
> +
> +	i2s_2ch_0: i2s@ff350000 {
> +		compatible = "rockchip,rk3308-i2s", "rockchip,rk3066-i2s";
> +		reg = <0x0 0xff350000 0x0 0x1000>;
> +		interrupts = <GIC_SPI 52 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&cru SCLK_I2S0_2CH>, <&cru HCLK_I2S0_2CH>;
> +		clock-names = "i2s_clk", "i2s_hclk";
> +		dmas = <&dmac1 8>, <&dmac1 9>;
> +		dma-names = "tx", "rx";
> +		resets = <&cru SRST_I2S0_2CH_M>, <&cru SRST_I2S0_2CH_H>;
> +		reset-names = "reset-m", "reset-h";

These resets don't seem to be defined in driver or binding?
Same for other i2s

> +		pinctrl-names = "default";
> +		pinctrl-0 = <&i2s_2ch_0_sclk
> +			     &i2s_2ch_0_lrck
> +			     &i2s_2ch_0_sdi
> +			     &i2s_2ch_0_sdo>;
> +		status = "disabled";
> +	};


> +
> +	mac: ethernet@ff4e0000 {
> +		compatible = "rockchip,rk3308-mac";

Was this support to the network driver already submitted?
Because I wasn't able to find it in the gmac driver.

> +		reg = <0x0 0xff4e0000 0x0 0x10000>;
> +		rockchip,grf = <&grf>;
> +		interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
> +		interrupt-names = "macirq";
> +		clocks = <&cru SCLK_MAC>, <&cru SCLK_MAC_RX_TX>,
> +			 <&cru SCLK_MAC_RX_TX>, <&cru SCLK_MAC_REF>,
> +			 <&cru SCLK_MAC>, <&cru ACLK_MAC>,
> +			 <&cru PCLK_MAC>, <&cru SCLK_MAC_RMII>;
> +		clock-names = "stmmaceth", "mac_clk_rx",
> +			      "mac_clk_tx", "clk_mac_ref",
> +			      "clk_mac_refout", "aclk_mac",
> +			      "pclk_mac", "clk_mac_speed";
> +		phy-mode = "rmii";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&rmii_pins &mac_refclk_12ma>;
> +		resets = <&cru SRST_MAC_A>;
> +		reset-names = "stmmaceth";
> +		status = "disabled";
> +	};


Heiko

Re: [PATCH 1/2] arm64: dts: rockchip: Add core dts for RK3308 SOC

[Andy Yan]

Hi Heiko:

Thanks for your kindly review.

On 10/18/19 7:30 AM, Heiko Stuebner wrote:
> Hi Andy,
>
> Am Donnerstag, 17. Oktober 2019, 05:04:49 CEST schrieb Andy Yan:
>
>> +	psci {
>> +		compatible = "arm,psci-1.0";
>> +		method = "smc";
>> +	};
> Please also provide a ATF implementation for the rk3308 :-)
> [Not a requirement for getting this merged, but it would be
> really cool to have sources for the full stack]


Tony's team has the plan to do it.

>> +
>> +	ramoops_mem: ramoops_mem {
>> +		reg = <0x0 0x110000 0x0 0xf0000>;
>> +		reg-names = "ramoops_mem";
>> +	};
>> +
>> +	ramoops: ramoops {
>> +		compatible = "ramoops";
>> +		record-size = <0x0 0x30000>;
>> +		console-size = <0x0 0xc0000>;
>> +		ftrace-size = <0x0 0x00000>;
>> +		pmsg-size = <0x0 0x00000>;
>> +		memory-region = <&ramoops_mem>;
>> +	};
> I think ramoops are more a per-board thing, like for the evb.
> As they'll require cooperation with bootloaders to not mangle that
> memory area. For this please also coordinate with Kever because
> I somehow remember we have u-boot sometimes at 0x100000.
>
I removed it in V2.
>> +	grf: grf@ff000000 {
>> +		compatible = "rockchip,rk3308-grf", "syscon", "simple-mfd";
> Please add a patch adding the rockchip,rk3308-grf compatible to
> Documentation/devicetree/bindings/soc/rockchip/grf.txt


Done

>
>> +		reg = <0x0 0xff000000 0x0 0x10000>;
>> +
>> +		reboot-mode {
>> +			compatible = "syscon-reboot-mode";
>> +			offset = <0x500>;
>> +			mode-bootloader = <BOOT_BL_DOWNLOAD>;
>> +			mode-loader = <BOOT_BL_DOWNLOAD>;
>> +			mode-normal = <BOOT_NORMAL>;
>> +			mode-recovery = <BOOT_RECOVERY>;
>> +			mode-fastboot = <BOOT_FASTBOOT>;
>> +		};
>> +	};
>> +
>> +	detect_grf: syscon@ff00b000 {
>> +		compatible = "syscon", "simple-mfd";
> 	compatible = "rockchip,rk3308-detect-grf", "syscon"
> + add the rk3308-detect-grf to the binding
Done
>> +		reg = <0x0 0xff00b000 0x0 0x1000>;
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +	};
>> +
>> +	core_grf: syscon@ff00c000 {
>> +		compatible = "syscon", "simple-mfd";
> same as detect_grf
Done
>
>> +		reg = <0x0 0xff00c000 0x0 0x1000>;
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +
>> +	};
>> +
>> +	i2c0: i2c@ff040000 {
>> +		compatible = "rockchip,rk3399-i2c";
> 	compatible = "rockchip,rk3308-i2c", "rockchip,rk3399-i2c";
> Same for all i2c controllers.
Done
>
>> +		reg = <0x0 0xff040000 0x0 0x1000>;
>> +		clocks = <&cru SCLK_I2C0>, <&cru PCLK_I2C0>;
>> +		clock-names = "i2c", "pclk";
>> +		interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&i2c0_xfer>;
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +		status = "disabled";
>> +	};
>
>> +	spi0: spi@ff120000 {
>> +		compatible = "rockchip,rk3308-spi", "rockchip,rk3066-spi";
>> +		reg = <0x0 0xff120000 0x0 0x1000>;
>> +		interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +		clocks = <&cru SCLK_SPI0>, <&cru PCLK_SPI0>;
>> +		clock-names = "spiclk", "apb_pclk";
>> +		dmas = <&dmac0 0>, <&dmac0 1>;
>> +		dma-names = "tx", "rx";
>> +		pinctrl-names = "default", "high_speed";
> there is no high_speed pinctrl defined for the Rockchip spi driver
> in mainline, so this part should go away in a first step.
> Same for the other spi controllers.
>
Removed
>> +		pinctrl-0 = <&spi0_clk &spi0_csn0 &spi0_miso &spi0_mosi>;
>> +		pinctrl-1 = <&spi0_clk_hs &spi0_csn0 &spi0_miso_hs &spi0_mosi_hs>;
>> +		status = "disabled";
>> +	};
>
>> +	rktimer: rktimer@ff1a0000 {
>> +		compatible = "rockchip,rk3288-timer";
> 	compatible = "rockchip,rk3308-timer", "rockchip,rk3288-timer";
Done
>
>> +		reg = <0x0 0xff1a0000 0x0 0x20>;
>> +		interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>;
>> +		clocks = <&cru PCLK_TIMER>, <&cru SCLK_TIMER0>;
>> +		clock-names = "pclk", "timer";
>> +	};
>
>
>> +	amba {
>> +		compatible = "arm,amba-bus";
> compatible = "simple-bus";

Done
>> +		#address-cells = <2>;
>> +		#size-cells = <2>;
>> +		ranges;
>> +
>> +		dmac0: dma-controller@ff2c0000 {
>> +			compatible = "arm,pl330", "arm,primecell";
>> +			reg = <0x0 0xff2c0000 0x0 0x4000>;
>> +			interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>,
>> +				     <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
>> +			#dma-cells = <1>;
>> +			clocks = <&cru ACLK_DMAC0>;
>> +			clock-names = "apb_pclk";
>> +			peripherals-req-type-burst;
> peripherals-req-type-burst is undocumented so likely some change to the
> dma driver not yet upstream?


  not upstream， so i remove it.

>> +		};
>> +
>> +		dmac1: dma-controller@ff2d0000 {
>> +			compatible = "arm,pl330", "arm,primecell";
>> +			reg = <0x0 0xff2d0000 0x0 0x4000>;
>> +			interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>,
>> +				     <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
>> +			#dma-cells = <1>;
>> +			clocks = <&cru ACLK_DMAC1>;
>> +			clock-names = "apb_pclk";
>> +			peripherals-req-type-burst;
>> +		};
>> +	};
>> +
>> +	i2s_2ch_0: i2s@ff350000 {
>> +		compatible = "rockchip,rk3308-i2s", "rockchip,rk3066-i2s";
>> +		reg = <0x0 0xff350000 0x0 0x1000>;
>> +		interrupts = <GIC_SPI 52 IRQ_TYPE_LEVEL_HIGH>;
>> +		clocks = <&cru SCLK_I2S0_2CH>, <&cru HCLK_I2S0_2CH>;
>> +		clock-names = "i2s_clk", "i2s_hclk";
>> +		dmas = <&dmac1 8>, <&dmac1 9>;
>> +		dma-names = "tx", "rx";
>> +		resets = <&cru SRST_I2S0_2CH_M>, <&cru SRST_I2S0_2CH_H>;
>> +		reset-names = "reset-m", "reset-h";
> These resets don't seem to be defined in driver or binding?
> Same for other i2s


Remove in v2

>
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&i2s_2ch_0_sclk
>> +			     &i2s_2ch_0_lrck
>> +			     &i2s_2ch_0_sdi
>> +			     &i2s_2ch_0_sdo>;
>> +		status = "disabled";
>> +	};
>
>> +
>> +	mac: ethernet@ff4e0000 {
>> +		compatible = "rockchip,rk3308-mac";
> Was this support to the network driver already submitted?
> Because I wasn't able to find it in the gmac driver.


I remove mac in v2.

>
>> +		reg = <0x0 0xff4e0000 0x0 0x10000>;
>> +		rockchip,grf = <&grf>;
>> +		interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
>> +		interrupt-names = "macirq";
>> +		clocks = <&cru SCLK_MAC>, <&cru SCLK_MAC_RX_TX>,
>> +			 <&cru SCLK_MAC_RX_TX>, <&cru SCLK_MAC_REF>,
>> +			 <&cru SCLK_MAC>, <&cru ACLK_MAC>,
>> +			 <&cru PCLK_MAC>, <&cru SCLK_MAC_RMII>;
>> +		clock-names = "stmmaceth", "mac_clk_rx",
>> +			      "mac_clk_tx", "clk_mac_ref",
>> +			      "clk_mac_refout", "aclk_mac",
>> +			      "pclk_mac", "clk_mac_speed";
>> +		phy-mode = "rmii";
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&rmii_pins &mac_refclk_12ma>;
>> +		resets = <&cru SRST_MAC_A>;
>> +		reset-names = "stmmaceth";
>> +		status = "disabled";
>> +	};
>
> Heiko
>
>
>
>

Re: [PATCH 2/2] arm64: dts: rockchip: Add basic dts for RK3308 EVB

[Heiko Stuebner]

Hi Andy,

Am Donnerstag, 17. Oktober 2019, 05:05:20 CEST schrieb Andy Yan:
> diff --git a/Documentation/devicetree/bindings/arm/rockchip.yaml b/Documentation/devicetree/bindings/arm/rockchip.yaml
> index c82c5e57d44c..b680c4b8b2c9 100644
> --- a/Documentation/devicetree/bindings/arm/rockchip.yaml
> +++ b/Documentation/devicetree/bindings/arm/rockchip.yaml
> @@ -447,6 +447,11 @@ properties:
>            - const: rockchip,r88
>            - const: rockchip,rk3368
>  
> +      - description: Rockchip RK3308 Evaluation board
> +        items:
> +          - const: rockchip,rk3308-evb
> +          - const: rockchip,rk3308
> +
>        - description: Rockchip RK3228 Evaluation board
>          items:
>            - const: rockchip,rk3228-evb

Rob likes the binding addition to be a separate patch.

> +	vdd_log: vdd_core: vdd-core {
> +		compatible = "pwm-regulator";
> +		pwms = <&pwm0 0 5000 1>;
> +		regulator-name = "vdd_core";
> +		regulator-min-microvolt = <827000>;
> +		regulator-max-microvolt = <1340000>;
> +		regulator-init-microvolt = <1015000>;
> +		regulator-early-min-microvolt = <1015000>;
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-settling-time-up-us = <250>;
> +		status = "okay";

It's a board-regulator, so always "okay", no need for a status.

In general for regulators, please create an actual regulator tree, with
correctly modelled supply-chains following the naming according
to the board schematics. See for example rk3399-gru for a nice example.

> +	};
> +
> +	vdd_1v0: vdd-1v0 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vdd_1v0";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <1000000>;
> +		regulator-max-microvolt = <1000000>;

As noted above, missing vin-supply

> +	};
> +

> +	vccio_flash: vccio-flash {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vccio_flash";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <1800000>;
> +		regulator-max-microvolt = <1800000>;
> +	};
> +
> +	vcc_phy: vcc-phy-regulator {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc_phy";
> +		regulator-always-on;
> +		regulator-boot-on;

This is the classic example of not following the schematics.
I.e. no Rockchip board I know has a regulator named "vcc_phy"
that is completely unconnected, yet all boards in the vendor tree
have this regulator ;-) ... so as I said, please follow the schematics.

> +	};
> +
> +	vbus_host: vbus-host-regulator {
> +		compatible = "regulator-fixed";
> +		enable-active-high;
> +		gpio = <&gpio0 RK_PC5 GPIO_ACTIVE_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&usb_drv>;
> +		regulator-name = "vbus_host";
> +	};
> +};
> +


Thanks
Heiko