| Message ID | 20200227180630.166982-1-t.schramm@manjaro.org |
|---|---|
| Headers | show |
| Series | Add support for the pine64 Pinebook Pro | expand |
Hi Tobias, Am Donnerstag, 27. Februar 2020, 19:06:30 CET schrieb Tobias Schramm: > This commit adds initial dt support for the rk3399 based Pinebook Pro. > > Signed-off-by: Tobias Schramm <t.schramm@manjaro.org> > + chosen { > + bootargs = "earlycon=uart8250,mmio32,0xff1a0000"; hmm, bootargs in a mainline dt seem awkward (argument about dt not being a place for configuration) ... so please drop that ... stdout-path can of course stay. > + stdout-path = "serial2:1500000n8"; > + }; > + > + leds { node sorting preference is by address for foo@bar nodes and alphabetically for everything else, so - backlight - edp-panel - gpio-key-lid .... > + compatible = "gpio-leds"; > + pinctrl-names = "default"; > + pinctrl-0 = <&pwrled_gpio &slpled_gpio>; > + > + green-led { > + color = <LED_COLOR_ID_GREEN>; > + default-state = "off"; > + function = LED_FUNCTION_POWER; > + gpios = <&gpio0 RK_PB3 GPIO_ACTIVE_HIGH>; > + label = "green:disk-activity"; > + linux,default-trigger = "mmc2"; hmm, LED_FUNCTION_POWER but trigger for mmc2 ? So if there is no activity on the LED it looks to be off? > + }; > + > + red-led { > + color = <LED_COLOR_ID_RED>; > + default-state = "off"; > + function = LED_FUNCTION_STANDBY; > + gpios = <&gpio0 RK_PA2 GPIO_ACTIVE_HIGH>; > + label = "red:standby"; > + panic-indicator; > + retain-state-suspended; > + }; > + }; > + > + /* Use separate nodes for gpio-keys to allow for selective deactivation nit: /* * Use separate .... > + * of wakeup sources without disabling the whole key Also can you explain the problem a bit? If there is a deficit in the input subsystem regarding wakeup events, dt is normally not the place to work around things [we're supposed to be OS independent] > + */ > + gpio-key-power { > + compatible = "gpio-keys"; > + pinctrl-names = "default"; > + pinctrl-0 = <&pwrbtn_gpio>; > + > + power { > + debounce-interval = <20>; > + gpios = <&gpio0 RK_PA5 GPIO_ACTIVE_LOW>; > + label = "Power"; > + linux,code = <KEY_POWER>; > + wakeup-source; > + }; > + }; > + > + gpio-key-lid { > + compatible = "gpio-keys"; > + pinctrl-names = "default"; > + pinctrl-0 = <&lidbtn_gpio>; > + > + lid { > + debounce-interval = <20>; > + gpios = <&gpio1 RK_PA1 GPIO_ACTIVE_LOW>; > + label = "Lid"; > + linux,code = <SW_LID>; > + linux,input-type = <EV_SW>; > + wakeup-event-action = <EV_ACT_DEASSERTED>; > + wakeup-source; > + }; > + }; > + > + /* first 128k(0xff8d0000~0xff8f0000) for ddr and ATF */ > + sram@ff8d0000 { > + compatible = "mmio-sram"; > + reg = <0x0 0xff8d0000 0x0 0x20000>; /* 128k */ > + }; (1) The sram would be soc property, so shouldn't be in a board-dts (2) nobody is using the sram? (3) you say 0xff8d0000~0xff8f0000 but then provide the same memory to the kernel? ATF will likely protect that memory from unsecure access. (not necessarily the old BSP binary-ATF though) So I'd suggest dropping the sram for now. > + > + edp_panel: edp-panel { > + compatible = "boe,nv140fhmn49", "simple-panel"; > + backlight = <&backlight>; > + > + enable-delay-ms = <20>; > + enable-gpios = <&gpio1 RK_PA0 GPIO_ACTIVE_HIGH>; > + pinctrl-names = "default"; > + pinctrl-0 = <&panel_en_gpio>; > + > + power-supply = <&vcc3v3_panel>; > + prepare-delay-ms = <20>; > + status = "okay"; edp-panel is a board-node and therefore default status okay > + > + ports { > + #address-cells = <0x01>; > + #size-cells = <0x00>; > + port@0 { > + panel_in_edp: endpoint@0 { > + remote-endpoint = <&edp_out_panel>; > + }; > + }; > + }; > + }; > + > + backlight: edp-backlight { > + compatible = "pwm-backlight"; > + brightness-levels = < > + 0 1 2 3 4 5 6 7 > + 8 9 10 11 12 13 14 15 > + 16 17 18 19 20 21 22 23 > + 24 25 26 27 28 29 30 31 > + 32 33 34 35 36 37 38 39 > + 40 41 42 43 44 45 46 47 > + 48 49 50 51 52 53 54 55 > + 56 57 58 59 60 61 62 63 > + 64 65 66 67 68 69 70 71 > + 72 73 74 75 76 77 78 79 > + 80 81 82 83 84 85 86 87 > + 88 89 90 91 92 93 94 95 > + 96 97 98 99 100 101 102 103 > + 104 105 106 107 108 109 110 111 > + 112 113 114 115 116 117 118 119 > + 120 121 122 123 124 125 126 127 > + 128 129 130 131 132 133 134 135 > + 136 137 138 139 140 141 142 143 > + 144 145 146 147 148 149 150 151 > + 152 153 154 155 156 157 158 159 > + 160 161 162 163 164 165 166 167 > + 168 169 170 171 172 173 174 175 > + 176 177 178 179 180 181 182 183 > + 184 185 186 187 188 189 190 191 > + 192 193 194 195 196 197 198 199 > + 200 201 202 203 204 205 206 207 > + 208 209 210 211 212 213 214 215 > + 216 217 218 219 220 221 222 223 > + 224 225 226 227 228 229 230 231 > + 232 233 234 235 236 237 238 239 > + 240 241 242 243 244 245 246 247 > + 248 249 250 251 252 253 254 255>; > + default-brightness-level = <200>; pwm-backlight can now calculate default brightness-levels, so you don't need the table and default-brightness-level. > + power-supply = <&vcc_12v>; > + pwms = <&pwm0 0 740740 0>; > + status = "okay"; same okay comment as above > + }; > + /* Audio components */ > + speaker_amp: speaker-amplifier { > + compatible = "simple-audio-amplifier"; > + enable-gpios = <&gpio4 RK_PD3 GPIO_ACTIVE_HIGH>; > + sound-name-prefix = "Speaker Amplifier"; > + status = "okay"; same okay comment as above [and I guess I should stop repeating this for all following status=okay in board nodes ;-) ] > + VCC-supply = <&pa_5v>; > + }; > + > + es8316-sound { > + compatible = "simple-audio-card"; > + pinctrl-names = "default"; > + pinctrl-0 = <&hp_det_gpio>; > + simple-audio-card,name = "rockchip,es8316-codec"; > + simple-audio-card,format = "i2s"; > + simple-audio-card,mclk-fs = <256>; > + > + simple-audio-card,widgets = > + "Microphone", "Mic Jack", > + "Headphone", "Headphones", > + "Speaker", "Speaker"; > + simple-audio-card,routing = > + "MIC1", "Mic Jack", > + "Headphones", "HPOL", > + "Headphones", "HPOR", > + "Speaker Amplifier INL", "HPOL", > + "Speaker Amplifier INR", "HPOR", > + "Speaker", "Speaker Amplifier OUTL", > + "Speaker", "Speaker Amplifier OUTR"; > + > + simple-audio-card,hp-det-gpio = <&gpio0 RK_PB0 GPIO_ACTIVE_LOW>; > + simple-audio-card,aux-devs = <&speaker_amp>; > + simple-audio-card,pin-switches = "Speaker"; > + status = "okay"; > + > + simple-audio-card,cpu { > + sound-dai = <&i2s1>; > + }; > + > + simple-audio-card,codec { > + sound-dai = <&es8316>; > + }; > + }; > + > + /* Power tree */ I really like clean power-trees, so thanks for probably digging through the schematics to get this right. [...] > +&cluster1_opp { > + opp08 { > + opp-hz = /bits/ 64 <2000000000>; > + opp-microvolt = <1300000>; Where does this operating point come from. The opp-table Rockchip specified for the stock-rk3399 ends at 1.8Ghz@1.2V and the OP1 variant only has a 2.016Ghz@1.25V . Adding overclocked cou rates to the DT we ship in the mainline > + }; > +}; > + > +&cdn_dp { > + status = "okay"; > + extcon = <&fusb0>; The fusb-extcon is Rockchip-BSP voodoo. This should use the kernel's type-c framework, which in turn is depending on the cros-ec-pd stuff from rk3399-gru also moving to the type-c framework ( > +}; > + > +/* CPU */ > +&cpu_alert0 { > + temperature = <80000>; > +}; > + > +&cpu_alert1 { > + temperature = <95000>; > +}; > + > +&cpu_crit { > + temperature = <100000>; > +}; Same issue for the temperatures. You're increasing the max allowed temperature and so may decrease the life expectancy of devices. The only other board-level changes for temperatures are decreasing them to actually prevent thermal issues. > +&i2c0 { > + clock-frequency = <400000>; > + i2c-scl-rising-time-ns = <168>; > + i2c-scl-falling-time-ns = <4>; > + status = "okay"; > + > + rk808: pmic@1b { > + compatible = "rockchip,rk808"; > + reg = <0x1b>; > + #clock-cells = <1>; > + clock-output-names = "xin32k", "rk808-clkout2"; > + interrupt-parent = <&gpio3>; > + interrupts = <10 IRQ_TYPE_LEVEL_LOW>; > + pinctrl-names = "default"; > + pinctrl-0 = <&pmic_int_l_gpio>; > + rockchip,system-power-controller; > + wakeup-source; > + > + vddio-supply = <&vcc_3v0>; where does this come from? Aka it's not specified in the dt-binding (though the example falsely uses it) and also not referenced in the driver. > + > + vdd_cpu_b: regulator@40 { > + compatible = "silergy,syr827"; > + reg = <0x40>; > + fcs,suspend-voltage-selector = <1>; > + pinctrl-names = "default"; > + pinctrl-0 = <&vsel1_gpio>; > + regulator-always-on; > + regulator-boot-on; > + regulator-compatible = "fan53555-reg"; > + regulator-min-microvolt = <712500>; > + regulator-max-microvolt = <1500000>; > + regulator-name = "vdd_cpu_b"; > + regulator-ramp-delay = <1000>; > + vin-supply = <&vcc_1v8>; > + vsel-gpios = <&gpio1 RK_PC1 GPIO_ACTIVE_HIGH>; not part of the regulator binding nor driver > + > + regulator-state-mem { > + regulator-off-in-suspend; > + }; > + }; > + > + vdd_gpu: regulator@41 { > + compatible = "silergy,syr828"; > + reg = <0x41>; > + fcs,suspend-voltage-selector = <1>; > + pinctrl-names = "default"; > + pinctrl-0 = <&vsel2_gpio>; > + regulator-always-on; > + regulator-boot-on; > + regulator-compatible = "fan53555-reg"; > + regulator-min-microvolt = <712500>; > + regulator-max-microvolt = <1500000>; > + regulator-name = "vdd_gpu"; > + regulator-ramp-delay = <1000>; > + vin-supply = <&vcc_1v8>; > + vsel-gpios = <&gpio1 RK_PB6 GPIO_ACTIVE_HIGH>; same > + > + regulator-state-mem { > + regulator-off-in-suspend; > + }; > + }; > +}; [...] > +&sdio0 { > + bus-width = <4>; > + cap-sd-highspeed; > + cap-sdio-irq; > + disable-wp; > + keep-power-in-suspend; > + mmc-pwrseq = <&sdio_pwrseq>; > + non-removable; > + num-slots = <1>; num-slots got removed a while ago > + pinctrl-names = "default"; > + pinctrl-0 = <&sdio0_bus4 &sdio0_cmd &sdio0_clk>; > + sd-uhs-sdr104; > + status = "okay"; > + supports-sdio; not part of the binding > +&tcphy0 { > + extcon = <&fusb0>; that is Rockchip voodoo. The fusb302 in mainline does not (and should not) export an extcon for status informations. Instead this should use the type-c framework. > + status = "okay"; > +}; > + > +&tcphy0_dp { > + port { > + tcphy0_typec_dp: endpoint { > + remote-endpoint = <&usbc_dp>; > + }; > + }; > +}; > + > +&tcphy0_usb3 { > + port { > + tcphy0_typec_ss: endpoint { > + remote-endpoint = <&usbc_ss>; > + }; > + }; > +}; [...] > +&u2phy1 { > + status = "okay"; > + > + u2phy1_otg: otg-port { > + status = "okay"; > + }; > + > + u2phy1_host: host-port { > + phy-supply = <&vcc5v0_otg>; > + status = "okay"; > + }; > +}; > + > + nit: double empty line > +&uart0 { Thanks Heiko
Hi Heiko, thanks for the review. I'll implement the changes and send a v2. >> + compatible = "gpio-leds"; >> + pinctrl-names = "default"; >> + pinctrl-0 = <&pwrled_gpio &slpled_gpio>; >> + >> + green-led { >> + color = <LED_COLOR_ID_GREEN>; >> + default-state = "off"; >> + function = LED_FUNCTION_POWER; >> + gpios = <&gpio0 RK_PB3 GPIO_ACTIVE_HIGH>; >> + label = "green:disk-activity"; >> + linux,default-trigger = "mmc2"; > hmm, LED_FUNCTION_POWER but trigger for mmc2 ? > So if there is no activity on the LED it looks to be off? I see why this is looking weird. It does not make a whole lot of sense at the moment and I'll change that for v2. However I have a patch in the making that adds "-inverted" variants for all triggers so the power LED can be turned of briefly to indicate mmc activity. Not sure wether people will like it or not but I'll try it as a RFC. >> + * of wakeup sources without disabling the whole key > Also can you explain the problem a bit? If there is a deficit in the input > subsystem regarding wakeup events, dt is normally not the place to work > around things [we're supposed to be OS independent] The issue is that some users wanted to be able to control the wakeup functionality of the keys separately via sysfs. That does not seem to be possible when combining both keys into one gpio-keys node. A more detailed explanation of the issue can be found at [1]. >> +&i2c0 { >> + clock-frequency = <400000>; >> + i2c-scl-rising-time-ns = <168>; >> + i2c-scl-falling-time-ns = <4>; >> + status = "okay"; >> + >> + rk808: pmic@1b { >> + compatible = "rockchip,rk808"; >> + reg = <0x1b>; >> + #clock-cells = <1>; >> + clock-output-names = "xin32k", "rk808-clkout2"; >> + interrupt-parent = <&gpio3>; >> + interrupts = <10 IRQ_TYPE_LEVEL_LOW>; >> + pinctrl-names = "default"; >> + pinctrl-0 = <&pmic_int_l_gpio>; >> + rockchip,system-power-controller; >> + wakeup-source; >> + >> + vddio-supply = <&vcc_3v0>; > where does this come from? Aka it's not specified in the dt-binding > (though the example falsely uses it) and also not referenced in the driver. This does likely come from the BSP dts. Seems I missed it while checking bindings. Thanks again for the review, Tobias [1] https://gitlab.manjaro.org/tsys/linux-pinebook-pro/issues/5
Hi Tobias, Am Freitag, 28. Februar 2020, 15:57:10 CET schrieb Tobias Schramm: > thanks for the review. I'll implement the changes and send a v2. > > >> + * of wakeup sources without disabling the whole key > > Also can you explain the problem a bit? If there is a deficit in the input > > subsystem regarding wakeup events, dt is normally not the place to work > > around things [we're supposed to be OS independent] > > The issue is that some users wanted to be able to control the wakeup > functionality of the keys separately via sysfs. That does not seem to be > possible when combining both keys into one gpio-keys node. A more > detailed explanation of the issue can be found at [1]. ok ... but that is really strange, because looking at gpio-keys.c I see it checking the individual button wakeup-property before setting the irq-wake in gpio_keys_enable_wakeup() . Ah, but I guess manually disabling/enabling wakeup via sysfs only works for the whole device and all wakeup buttons. In general this sounds more like a gpio-keys deficit, but in the end we can keep the separate gpio-key nodes here, they don't violate any dt-bindings ;-) . Heiko