Hi Krzysztof, Kever, Heiko and others,
On 10/27/22 16:58, Krzysztof Kozlowski wrote:
> On 26/10/2022 20:53, Johan Jonker wrote:
>> Add basic rk3128 support.
>>
>
> Thank you for your patch. There is something to discuss/improve.
Thank you for your review.
Some more questions/comments below.
>
>> +#include <dt-bindings/clock/rk3128-cru.h>
>> +#include <dt-bindings/gpio/gpio.h>
>> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>> +#include <dt-bindings/interrupt-controller/irq.h>
>> +#include <dt-bindings/pinctrl/rockchip.h>
>> +
>> +/ {
>> + compatible = "rockchip,rk3128";
>> + interrupt-parent = <&gic>;
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> +
>> + aliases {
>> + gpio0 = &gpio0;
>> + gpio1 = &gpio1;
>> + gpio2 = &gpio2;
>> + gpio3 = &gpio3;
Is gpio OK here?
>> + i2c0 = &i2c0;
>> + i2c1 = &i2c1;
>> + i2c2 = &i2c2;
>> + i2c3 = &i2c3;
>> + spi0 = &spi0;
>> + serial0 = &uart0;
>> + serial1 = &uart1;
>> + serial2 = &uart2;
>
> Bus aliases are board specific and represent what is actually available
> on headers/pins etc. These do not belong to SoC DTSI.
I just follow current Rockchip DT common practice.
Do we need to change all Rockchip boards?
Would like to hear from Heiko what's the plan here?
Syncing to U-boot is already a mess...
So far only instructions/changes and discussion about mmc nodes.
Can Rockchip specific rules be publicized in a central place?
===
mmc aliases on reg order, availability and without number gap.
===
Heiko's sort rules:
compatible
reg
interrupts
[alphabetical]
status [if needed]
===
My incomplete list:
For nodes:
If exists on top: model, compatible and chosen.
Sort things without reg alphabetical first,
then sort the rest by reg address.
Inside nodes:
If exists on top: compatible, reg and interrupts.
In alphabetical order the required properties.
Then in alphabetical order the other properties.
And as last things that start with '#' in alphabetical order.
Add status below all other properties for soc internal components with
any board-specifics.
Keep an empty line between properties and nodes.
Exceptions:
Sort pinctrl-0 above pinctrl-names, so it stays in line with clock-names
and dma-names.
Sort simple-audio-card,name above other simple-audio-card properties.
Sort regulator-name above other regulator properties.
Sort regulator-min-microvolt above regulator-max-microvolt.
>
>> + };
>> +
>> + arm-pmu {
>> + compatible = "arm,cortex-a7-pmu";
>> + interrupts = <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>,
>> + <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>,
>> + <GIC_SPI 78 IRQ_TYPE_LEVEL_HIGH>,
>> + <GIC_SPI 79 IRQ_TYPE_LEVEL_HIGH>;
>> + interrupt-affinity = <&cpu0>, <&cpu1>, <&cpu2>, <&cpu3>;
>> + };
>> +
>> + cpus {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + cpu0: cpu@f00 {
>> + device_type = "cpu";
>> + compatible = "arm,cortex-a7";
>> + reg = <0xf00>;
>> + clock-latency = <40000>;
>> + clocks = <&cru ARMCLK>;
>> + operating-points = <
>> + /* KHz uV */
>> + 816000 1000000
>> + >;
>
> Why not operating-points-v2?
rk3128 doesn't have a tsadc.
As long as this thermal stuff is not implemented with drivers and regulators I would prefer to keep it basic in the DT for now.
Just keep things simple for now till someone with hardware can fix that.
https://github.com/rockchip-linux/kernel/blob/develop-4.4/arch/arm/boot/dts/rk312x.dtsi#L315
tsadc: tsadc {
compatible = "rockchip,rk3126-tsadc-virtual";
nvmem-cells = <&cpu_leakage>;
nvmem-cell-names = "cpu_leakage";
#thermal-sensor-cells = <1>;
status = "disabled";
};
>
>> + #cooling-cells = <2>; /* min followed by max */
>> + };
>> +
>> + cpu1: cpu@f01 {
>> + device_type = "cpu";
>> + compatible = "arm,cortex-a7";
>> + reg = <0xf01>;
>> + };
>> +
>> + cpu2: cpu@f02 {
>> + device_type = "cpu";
>> + compatible = "arm,cortex-a7";
>> + reg = <0xf02>;
>> + };
>> +
>> + cpu3: cpu@f03 {
>> + device_type = "cpu";
>> + compatible = "arm,cortex-a7";
>> + reg = <0xf03>;
>> + };
>> + };
>> +
>> + timer {
>> + compatible = "arm,armv7-timer";
Original 2 interrupts:
>> + interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
>> + <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
Could someone help here with the correct interrupts properties?
This was later changed to 4 interrupts:
interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
<GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
<GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
<GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
See:
arm: dts: enable arm arch virtual timer for rk312x
https://github.com/rockchip-linux/kernel/commit/405105146fd8a25ef3c9c46d18895ad243a6db2d
===
How to calculate the number?
Please advise? Thanks!
>From rk3128 TRM show only 3 interrupts: ????
PPI
27 VIRTUAL TIMER High level
29 SECURE PHYSICAL TIMER High level
30 NON-SECURE PHY TIMER High level
>> + arm,cpu-registers-not-fw-configured;
>> + clock-frequency = <24000000>;
>> + };
>> +
>> + xin24m: oscillator {
>> + compatible = "fixed-clock";
>> + clock-frequency = <24000000>;
>> + clock-output-names = "xin24m";
>> + #clock-cells = <0>;
>> + };
>> +
>> + pmu: syscon@100a0000 {
>> + compatible = "rockchip,rk3128-pmu", "syscon", "simple-mfd";
>> + reg = <0x100a0000 0x1000>;
>> + };
>> +
>> + gic: interrupt-controller@10139000 {
>> + compatible = "arm,cortex-a7-gic";
>> + reg = <0x10139000 0x1000>,
>> + <0x1013a000 0x1000>,
>> + <0x1013c000 0x2000>,
>> + <0x1013e000 0x2000>;
>> + interrupts = <GIC_PPI 9 0xf04>;
How can this number be 9 if the lowest PPI IRQ ID is:
27 VIRTUAL TIMER High level
Can someone help here? Thanks!
>
> f04 looks like a mask, so use standard defines for it.
OK
Will check which defines ends up with 0xf04.
interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>; ??
>
>> + interrupt-controller;
>> + #interrupt-cells = <3>;
>> + #address-cells = <0>;
>> + };
>> +
>> + usb_otg: usb@10180000 {
>> + compatible = "rockchip,rk3128-usb", "rockchip,rk3066-usb", "snps,dwc2";
>> + reg = <0x10180000 0x40000>;
>> + interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&cru HCLK_OTG>;
>> + clock-names = "otg";
>> + dr_mode = "otg";
>> + phys = <&usb2phy_otg>;
>> + phy-names = "usb2-phy";
>> + status = "disabled";
>> + };
>> +
>> + usb_host_ehci: usb@101c0000 {
>> + compatible = "generic-ehci";
>> + reg = <0x101c0000 0x20000>;
>> + interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
>> + phys = <&usb2phy_host>;
>> + phy-names = "usb";
>> + status = "disabled";
>> + };
>> +
>> + usb_host_ohci: usb@101e0000 {
>> + compatible = "generic-ohci";
>> + reg = <0x101e0000 0x20000>;
>> + interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
>> + phys = <&usb2phy_host>;
>> + phy-names = "usb";
>> + status = "disabled";
>> + };
>> +
>> + sdmmc: mmc@10214000 {
>> + compatible = "rockchip,rk3128-dw-mshc", "rockchip,rk3288-dw-mshc";
>> + reg = <0x10214000 0x4000>;
>> + interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&cru HCLK_SDMMC>, <&cru SCLK_SDMMC>,
>> + <&cru SCLK_SDMMC_DRV>, <&cru SCLK_SDMMC_SAMPLE>;
>> + clock-names = "biu", "ciu", "ciu-drive", "ciu-sample";
>> + dmas = <&pdma 10>;
>> + dma-names = "rx-tx";
>> + fifo-depth = <256>;
>> + max-frequency = <150000000>;
>> + resets = <&cru SRST_SDMMC>;
>> + reset-names = "reset";
>> + status = "disabled";
>> + };
>> +
>> + sdio: mmc@10218000 {
>> + compatible = "rockchip,rk3128-dw-mshc", "rockchip,rk3288-dw-mshc";
>> + reg = <0x10218000 0x4000>;
>> + interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&cru HCLK_SDIO>, <&cru SCLK_SDIO>,
>> + <&cru SCLK_SDIO_DRV>, <&cru SCLK_SDIO_SAMPLE>;
>> + clock-names = "biu", "ciu", "ciu-drive", "ciu-sample";
>> + dmas = <&pdma 11>;
>> + dma-names = "rx-tx";
>> + fifo-depth = <256>;
>> + max-frequency = <150000000>;
>> + resets = <&cru SRST_SDIO>;
>> + reset-names = "reset";
>> + status = "disabled";
>> + };
>> +
>> + emmc: mmc@1021c000 {
>> + compatible = "rockchip,rk3128-dw-mshc", "rockchip,rk3288-dw-mshc";
>> + reg = <0x1021c000 0x4000>;
>> + interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&cru HCLK_EMMC>, <&cru SCLK_EMMC>,
>> + <&cru SCLK_EMMC_DRV>, <&cru SCLK_EMMC_SAMPLE>;
>> + clock-names = "biu", "ciu", "ciu-drive", "ciu-sample";
>> + dmas = <&pdma 12>;
>> + dma-names = "rx-tx";
>> + fifo-depth = <256>;
>> + max-frequency = <150000000>;
>> + resets = <&cru SRST_EMMC>;
>> + reset-names = "reset";
>> + status = "disabled";
>> + };
>> +
>> + nfc: nand-controller@10500000 {
>> + compatible = "rockchip,rk3128-nfc", "rockchip,rk2928-nfc";
>> + reg = <0x10500000 0x4000>;
>> + interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&cru HCLK_NANDC>, <&cru SCLK_NANDC>;
>> + clock-names = "ahb", "nfc";
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&flash_ale &flash_bus8 &flash_cle &flash_cs0
>> + &flash_dqs &flash_rdn &flash_rdy &flash_wrn>;
>> + status = "disabled";
>> + };
>> +
>> + cru: clock-controller@20000000 {
>> + compatible = "rockchip,rk3128-cru";
>> + reg = <0x20000000 0x1000>;
>> + clocks = <&xin24m>;
>> + clock-names = "xin24m";
>> + rockchip,grf = <&grf>;
>> + #clock-cells = <1>;
>> + #reset-cells = <1>;
>> + assigned-clocks = <&cru PLL_GPLL>;
>> + assigned-clock-rates = <594000000>;
>> + };
>> +
>> + grf: syscon@20008000 {
>> + compatible = "rockchip,rk3128-grf", "syscon", "simple-mfd";
>> + reg = <0x20008000 0x1000>;
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> +
>> + usb2phy: usb2phy@17c {
>
> Node names should be generic.
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
You are absolutely correct. Except for Rockchip usb2phy nodes ....
#phy-cells is located in a sub node, so we keep as it is... ;)
dt-bindings: phy: rename phy nodename in phy-rockchip-inno-usb2.yaml
https://lore.kernel.org/all/20210601164800.7670-2-jbx6244@xxxxxxxxx/
>
>> + compatible = "rockchip,rk3128-usb2phy";
>> + reg = <0x017c 0x0c>;
>> + clocks = <&cru SCLK_OTGPHY0>;
>> + clock-names = "phyclk";
>> + clock-output-names = "usb480m_phy";
>> + #clock-cells = <0>;
>> + status = "disabled";
>> +
>
>
> Best regards,
> Krzysztof
>
Kind regards,
Johan Jonker
