Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1384593 > unrolled thread
| Started by | Heiko Stübner <heiko@sntech.de> |
|---|---|
| First post | 2016-04-22 00:10 +0200 |
| Last post | 2016-04-22 06:30 +0200 |
| Articles | 4 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH] ARM64: dts: rockchip: add core dtsi file for RK3399 SoCs Heiko Stübner <heiko@sntech.de> - 2016-04-22 00:10 +0200
Re: [PATCH] ARM64: dts: rockchip: add core dtsi file for RK3399 SoCs Doug Anderson <dianders@chromium.org> - 2016-04-22 00:40 +0200
Re: [PATCH] ARM64: dts: rockchip: add core dtsi file for RK3399 SoCs Heiko Stübner <heiko@sntech.de> - 2016-04-22 00:50 +0200
Re: [PATCH] ARM64: dts: rockchip: add core dtsi file for RK3399 SoCs "Huang, Tao" <huangtao@rock-chips.com> - 2016-04-22 06:30 +0200
| From | Heiko Stübner <heiko@sntech.de> |
|---|---|
| Date | 2016-04-22 00:10 +0200 |
| Subject | Re: [PATCH] ARM64: dts: rockchip: add core dtsi file for RK3399 SoCs |
| Message-ID | <rquXo-88E-23@gated-at.bofh.it> |
Hi Jay,
Am Donnerstag, 21. April 2016, 11:58:12 schrieb Jianqun Xu:
> This patch adds rk3399.dtsi for rk3399 found on Rockchip
> RK3399 SoCs, also add rk3399-evb.dts for Rockchip RK3399
> Evaluation Board.
>
> Patch is tested on RK3399 evb.
>
> Signed-off-by: Jianqun Xu <jay.xu@rock-chips.com>
please split this into
- patch adding the dtsi
- patch adding the evb dts
- patch adding the new board to bindings/arm/rockchip.txt
more inline below
> ---
> arch/arm64/boot/dts/rockchip/Makefile | 1 +
> arch/arm64/boot/dts/rockchip/rk3399-evb.dts | 537 ++++++++
> arch/arm64/boot/dts/rockchip/rk3399.dtsi | 1757
> +++++++++++++++++++++++++++ 3 files changed, 2295 insertions(+)
> create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-evb.dts
> create mode 100644 arch/arm64/boot/dts/rockchip/rk3399.dtsi
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-evb.dts
> b/arch/arm64/boot/dts/rockchip/rk3399-evb.dts new file mode 100644
> index 0000000..4cb0028
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-evb.dts
> @@ -0,0 +1,537 @@
> +/*
> + * Copyright (c) 2016 Fuzhou Rockchip Electronics Co., Ltd
> + *
> + * This file is dual-licensed: you can use it either under the terms
> + * of the GPL or the X11 license, at your option. Note that this dual
> + * licensing only applies to this file, and not this project as a
> + * whole.
> + *
> + * a) This file is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of the
> + * License, or (at your option) any later version.
> + *
> + * This file is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * Or, alternatively,
> + *
> + * b) Permission is hereby granted, free of charge, to any person
> + * obtaining a copy of this software and associated documentation
> + * files (the "Software"), to deal in the Software without
> + * restriction, including without limitation the rights to use,
> + * copy, modify, merge, publish, distribute, sublicense, and/or
> + * sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following
> + * conditions:
> + *
> + * The above copyright notice and this permission notice shall be
> + * included in all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
> + * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
> + * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
> + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +/dts-v1/;
> +
> +#include <dt-bindings/pwm/pwm.h>
> +#include "rk3399.dtsi"
> +
> +/ {
> + model = "Rockchip RK3399 Evaluation Board";
> + compatible = "rockchip,evb", "rockchip,rk3399-evb";
> +
> + chosen {
> + bootargs = "console=uart,mmio32,0xff1a0000";
I'd think we'll want a
stdout-path = something
property here, instead of hard-coding bootargs.
[...]
> +&i2c4 {
> + status = "okay";
> + i2c-scl-rising-time-ns = <600>;
> + i2c-scl-falling-time-ns = <20>;
> +
> + gt9xx: gt9xx@14 {
> + compatible = "goodix,gt9xx";
same as Rob said for the ramoops, I don't see this one in the devicetree
bindings. Also gt9xx should instead specify an actual chip, not a chip-family.
See drivers/input/touchscreen/goodix.c and
Documentation/devicetree/bindings/input/touchscreen for supported chips and
the real devicetree bindings.
> + reg = <0x14>;
> + touch-gpio = <&gpio1 20 IRQ_TYPE_LEVEL_LOW>;
> + reset-gpio = <&gpio4 22 GPIO_ACTIVE_HIGH>;
> + max-x = <1200>;
> + max-y = <1900>;
> + tp-size = <911>;
> + tp-supply = <&vcc3v0_tp>;
> + };
> +};
[...]
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> b/arch/arm64/boot/dts/rockchip/rk3399.dtsi new file mode 100644
> index 0000000..7c3015c
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> @@ -0,0 +1,1757 @@
> +/*
> + * Copyright (c) 2016 Fuzhou Rockchip Electronics Co., Ltd
> + *
> + * This file is dual-licensed: you can use it either under the terms
> + * of the GPL or the X11 license, at your option. Note that this dual
> + * licensing only applies to this file, and not this project as a
> + * whole.
> + *
> + * a) This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of the
> + * License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * Or, alternatively,
> + *
> + * b) Permission is hereby granted, free of charge, to any person
> + * obtaining a copy of this software and associated documentation
> + * files (the "Software"), to deal in the Software without
> + * restriction, including without limitation the rights to use,
> + * copy, modify, merge, publish, distribute, sublicense, and/or
> + * sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following
> + * conditions:
> + *
> + * The above copyright notice and this permission notice shall be
> + * included in all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
> + * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
> + * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
> + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +#include <dt-bindings/clock/rk3399-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>
> +#include <dt-bindings/power/rk3399-power.h>
> +#include <dt-bindings/thermal/thermal.h>
> +
> +/ {
[...]
> + sdhci: sdhci@fe330000 {
> + compatible = "arasan,sdhci-5.1";
not 100% sure, but we might want a
compatible = "rockchip,rk3399-sdhci-5.1", "arasan,sdhci-5.1";
allowing us to get more specific, if implementation oddities surface later.
> + reg = <0x0 0xfe330000 0x0 0x10000>;
> + interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&cru SCLK_EMMC>, <&cru ACLK_EMMC>;
> + clock-names = "clk_xin", "clk_ahb";
> + phys = <&emmc_phy>;
> + phy-names = "phy_arasan";
> + status = "disabled";
> + };
> +
> + usb2phy: usb2phy {
> + compatible = "rockchip,rk3399-usb-phy";
this doesn't look like it got submitted yet.
Also, the newer socs (rk3399. rk3036, rk3228) seem to use a different usbphy
block than rk3288 and before (with a big bunch of new phy-related register
blocks I haven't looked at yet) - so this should probably get a new driver as
well and not be crammed into the current phy driver, which is for the older
picophy (or what it was called).
> + rockchip,grf = <&grf>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + usb2phy0: usb2-phy0 {
> + #phy-cells = <0>;
> + #clock-cells = <0>;
> + reg = <0xe458>;
> + };
When we're doing a new driver, could we please get rid of these subnodes and
instead access phys via something like
phys = <&usb2phy 0>;
> +
> + usb2phy1: usb2-phy1 {
> + #phy-cells = <0>;
> + #clock-cells = <0>;
> + reg = <0xe468>;
> + };
> + };
> +
> + usb_host0_echi: usb@fe380000 {
not "echi" please :-)
> + compatible = "generic-ehci";
> + reg = <0x0 0xfe380000 0x0 0x20000>;
> + interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>;
> + clock-names = "hclk_host0", "hclk_host0_arb";
> + phys = <&usb2phy0>;
> + phy-names = "usb2_phy0";
> + status = "disabled";
> + };
[...]
> + usbdrd3_0: usb@fe800000 {
> + compatible = "rockchip,dwc3";
is this in some tree already?
> + clocks = <&cru SCLK_USB3OTG0_REF>, <&cru SCLK_USB3OTG0_SUSPEND>,
> + <&cru ACLK_USB3OTG0>, <&cru ACLK_USB3_RKSOC_AXI_PERF>,
> + <&cru ACLK_USB3>, <&cru ACLK_USB3_GRF>;
> + clock-names = "clk_usb3otg0_ref", "clk_usb3otg0_suspend",
> + "aclk_usb3otg0", "aclk_usb3_rksoc_axi_perf",
> + "aclk_usb3", "aclk_usb3_grf";
> + #address-cells = <2>;
> + #size-cells = <2>;
> + ranges;
> + status = "disabled";
> + usbdrd_dwc3_0: dwc3 {
> + compatible = "snps,dwc3";
> + reg = <0x0 0xfe800000 0x0 0x100000>;
> + interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>;
> + dr_mode = "otg";
> + tx-fifo-resize;
> + snps,dis_enblslpm_quirk;
> + snps,phyif_utmi_16_bits;
> + snps,dis_u2_freeclk_exists_quirk;
> + snps,dis_del_phy_power_chg_quirk;
> + status = "disabled";
> + };
> + };
> +
> + usbdrd3_1: usb@fe900000 {
> + compatible = "rockchip,dwc3";
same here
> + clocks = <&cru SCLK_USB3OTG1_REF>, <&cru SCLK_USB3OTG1_SUSPEND>,
> + <&cru ACLK_USB3OTG1>, <&cru ACLK_USB3_RKSOC_AXI_PERF>,
> + <&cru ACLK_USB3>, <&cru ACLK_USB3_GRF>;
> + clock-names = "clk_usb3otg1_ref", "clk_usb3otg1_suspend",
> + "aclk_usb3otg1", "aclk_usb3_rksoc_axi_perf",
> + "aclk_usb3", "aclk_usb3_grf";
> + #address-cells = <2>;
> + #size-cells = <2>;
> + ranges;
> + status = "disabled";
> + usbdrd_dwc3_1: dwc3 {
> + compatible = "snps,dwc3";
> + reg = <0x0 0xfe900000 0x0 0x100000>;
> + interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
> + dr_mode = "otg";
> + tx-fifo-resize;
> + snps,dis_enblslpm_quirk;
> + snps,phyif_utmi_16_bits;
> + snps,dis_u2_freeclk_exists_quirk;
> + snps,dis_del_phy_power_chg_quirk;
> + status = "disabled";
> + };
> + };
> +
[...]
> + i2c1: i2c@ff110000 {
> + compatible = "rockchip,rk3399-i2c";
David respun the rk3399 i2c-support on tuesday, so this and the others below
are waiting on Wolfram to take a look.
> + reg = <0x0 0xff110000 0x0 0x1000>;
> + clocks = <&cru SCLK_I2C1>, <&cru PCLK_I2C1>;
> + clock-names = "i2c", "pclk";
> + interrupts = <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&i2c1_xfer>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + status = "disabled";
> + };
[...]
> + pmu: power-management@ff31000 {
address is missing "0" :-)
[...]
> + };
[...]
> + i2c0: i2c@ff3c0000 {
> + compatible = "rockchip,rk3399-i2c";
> + reg = <0x0 0xff3c0000 0x0 0x1000>;
> + clocks = <&pmucru SCLK_I2C0_PMU>, <&pmucru PCLK_I2C0_PMU>;
> + clock-names = "i2c", "pclk";
> + interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&i2c0_xfer>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + status = "disabled";
> + };
> +
> + i2c4: i2c@ff3d0000 {
> + compatible = "rockchip,rk3399-i2c";
> + reg = <0x0 0xff3d0000 0x0 0x1000>;
> + clocks = <&pmucru SCLK_I2C4_PMU>, <&pmucru PCLK_I2C4_PMU>;
> + clock-names = "i2c", "pclk";
> + interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&i2c4_xfer>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + status = "disabled";
> + };
> +
> + i2c8: i2c@ff3e0000 {
> + compatible = "rockchip,rk3399-i2c";
> + reg = <0x0 0xff3e0000 0x0 0x1000>;
> + clocks = <&pmucru SCLK_I2C8_PMU>, <&pmucru PCLK_I2C8_PMU>;
> + clock-names = "i2c", "pclk";
> + interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&i2c8_xfer>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + status = "disabled";
> + };
> + rga: rga@ff680000 {
> + compatible = "rockchip,rk3399-rga";
not yet accepted component, please leave out for now
> + reg = <0x0 0xff680000 0x0 0x10000>;
> + interrupts = <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-names = "rga";
> + clocks = <&cru ACLK_RGA>, <&cru HCLK_RGA>, <&cru SCLK_RGA_CORE>;
> + clock-names = "aclk", "hclk", "sclk";
> + resets = <&cru SRST_RGA_CORE>, <&cru SRST_A_RGA>, <&cru SRST_H_RGA>;
> + reset-names = "core", "axi", "ahb";
> + status = "disabled";
> + };
> +
[...]
> + gpu: gpu@ff9a0000 {
> + compatible = "arm,malit860",
> + "arm,malit86x",
> + "arm,malit8xx",
> + "arm,mali-midgard";
mali kernel-part is out-of-tree code with a unreviewed binding, so should not
be part of the mainline devicetree
> + reg = <0x0 0xff9a0000 0x0 0x10000>;
> + interrupts = <GIC_SPI 19 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 20 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-names = "GPU", "JOB", "MMU";
> +
> + clocks = <&cru ACLK_GPU>;
> + clock-names = "clk_mali";
> + #cooling-cells = <2>; /* min followed by max */
> + status = "disabled";
> + };
[...]
> + mipi_dsi: mipi@ff960000 {
> + compatible = "rockchip,rk3399-mipi-dsi", "snps,dw-mipi-dsi";
missing binding in the kernel
> + reg = <0x0 0xff960000 0x0 0x8000>;
> + interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&cru SCLK_MIPIDPHY_REF>, <&cru PCLK_MIPI_DSI0>,
> + <&cru SCLK_DPHY_TX0_CFG>;
> + clock-names = "ref", "pclk", "phy_cfg";
> + rockchip,grf = <&grf>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + status = "disabled";
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <1>;
> +
> + mipi_in: port {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + mipi_in_vopb: endpoint@0 {
> + reg = <0>;
> + remote-endpoint = <&vopb_out_mipi>;
> + };
> + mipi_in_vopl: endpoint@1 {
> + reg = <1>;
> + remote-endpoint = <&vopl_out_mipi>;
> + };
> + };
> + };
> + };
> +
> + edp: edp@ff970000 {
> + compatible = "rockchip,rk3399-edp";
missing binding in the kernel?
> + reg = <0x0 0xff970000 0x0 0x8000>;
> + interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&cru PCLK_EDP>, <&cru PCLK_EDP_CTRL>;
> + clock-names = "dp", "pclk";
> + resets = <&cru SRST_P_EDP_CTRL>;
> + reset-names = "dp";
> + rockchip,grf = <&grf>;
> + status = "disabled";
> + pinctrl-names = "default";
> + pinctrl-0 = <&edp_hpd>;
I think the hotplug detection is pretty optional, so should live in the board
files instead
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + edp_in: port@0 {
> + reg = <0>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + edp_in_vopb: endpoint@0 {
> + reg = <0>;
> + remote-endpoint = <&vopb_out_edp>;
> + };
> +
> + edp_in_vopl: endpoint@1 {
> + reg = <1>;
> + remote-endpoint = <&vopl_out_edp>;
> + };
> + };
> + };
> + };
[toc] | [next] | [standalone]
| From | Doug Anderson <dianders@chromium.org> |
|---|---|
| Date | 2016-04-22 00:40 +0200 |
| Message-ID | <rqvqq-8mO-17@gated-at.bofh.it> |
| In reply to | #1384593 |
Hi,
I didn't look as deeply as Heiko, but a few comments...
On Thu, Apr 21, 2016 at 3:02 PM, Heiko Stübner <heiko@sntech.de> wrote:
> Hi Jay,
>
> Am Donnerstag, 21. April 2016, 11:58:12 schrieb Jianqun Xu:
>> This patch adds rk3399.dtsi for rk3399 found on Rockchip
>> RK3399 SoCs, also add rk3399-evb.dts for Rockchip RK3399
>> Evaluation Board.
>>
>> Patch is tested on RK3399 evb.
>>
>> Signed-off-by: Jianqun Xu <jay.xu@rock-chips.com>
>
> please split this into
> - patch adding the dtsi
> - patch adding the evb dts
> - patch adding the new board to bindings/arm/rockchip.txt
>
> more inline below
Also don't forget to remove the controversial pmu bits for now (as
discussed earlier) so this can land while all those kinks are being
worked out.
>> + sdhci: sdhci@fe330000 {
>> + compatible = "arasan,sdhci-5.1";
>
> not 100% sure, but we might want a
> compatible = "rockchip,rk3399-sdhci-5.1", "arasan,sdhci-5.1";
>
> allowing us to get more specific, if implementation oddities surface later.
I agree with Heiko. This sounds very sane to me, too, and matches
previous discussions.
>> + reg = <0x0 0xfe330000 0x0 0x10000>;
>> + interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&cru SCLK_EMMC>, <&cru ACLK_EMMC>;
>> + clock-names = "clk_xin", "clk_ahb";
>> + phys = <&emmc_phy>;
>> + phy-names = "phy_arasan";
>> + status = "disabled";
>> + };
>> +
>> + usb2phy: usb2phy {
>> + compatible = "rockchip,rk3399-usb-phy";
>
> this doesn't look like it got submitted yet.
>
> Also, the newer socs (rk3399. rk3036, rk3228) seem to use a different usbphy
> block than rk3288 and before (with a big bunch of new phy-related register
> blocks I haven't looked at yet) - so this should probably get a new driver as
> well and not be crammed into the current phy driver, which is for the older
> picophy (or what it was called).
>
>
>> + rockchip,grf = <&grf>;
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + usb2phy0: usb2-phy0 {
>> + #phy-cells = <0>;
>> + #clock-cells = <0>;
>> + reg = <0xe458>;
>> + };
>
> When we're doing a new driver, could we please get rid of these subnodes and
> instead access phys via something like
>
> phys = <&usb2phy 0>;
From what I recall during the submission of the previous PHY Kishon
preferred the subnodes. I think I made a fool of myself in the last
discussion about this because I reported bugs in my downstream kernel
that didn't exist upstream, but if you want to read it you can see
here:
https://patchwork.kernel.org/patch/5474871/
I believe patch v6 used IDs like Heiko is suggesting and it turned to
subnodes in v7 based on Kishon's request. Since PHY code and bindings
are Kishon's call, I have a feeling his opinion will trump here.
>> +
>> + usb2phy1: usb2-phy1 {
>> + #phy-cells = <0>;
>> + #clock-cells = <0>;
>> + reg = <0xe468>;
>> + };
>> + };
>> +
>> + usb_host0_echi: usb@fe380000 {
>
> not "echi" please :-)
Just because it took me an extra reading to understand, he means turn
"echi" to "ehci".
>> + compatible = "generic-ehci";
>> + reg = <0x0 0xfe380000 0x0 0x20000>;
>> + interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>;
>> + clock-names = "hclk_host0", "hclk_host0_arb";
>> + phys = <&usb2phy0>;
>> + phy-names = "usb2_phy0";
>> + status = "disabled";
>> + };
>
> [...]
>
>> + usbdrd3_0: usb@fe800000 {
>> + compatible = "rockchip,dwc3";
>
> is this in some tree already?
I'm really surprised that there's not some generic fallback for
"dwc3-of-simple.c". I would have expected:
"rockchip,rk3399-dwc3", "synopsis,dwc3";
...but that doesn't appear to be in the bindings. Weird.
>> + i2c1: i2c@ff110000 {
>> + compatible = "rockchip,rk3399-i2c";
>
> David respun the rk3399 i2c-support on tuesday, so this and the others below
> are waiting on Wolfram to take a look.
I think it can work with the rk3288-i2c as a fallback, at least for
low speed stuff, right? Should this be:
compatible = "rockchip,rk3399-i2c", "rockchip,rk3288-i2c"
Looks like that was done for rk3368.
-Doug
[toc] | [prev] | [next] | [standalone]
| From | Heiko Stübner <heiko@sntech.de> |
|---|---|
| Date | 2016-04-22 00:50 +0200 |
| Message-ID | <rqvA6-8qC-1@gated-at.bofh.it> |
| In reply to | #1384617 |
Am Donnerstag, 21. April 2016, 15:38:22 schrieb Doug Anderson:
> Hi,
>
> I didn't look as deeply as Heiko, but a few comments...
>
> On Thu, Apr 21, 2016 at 3:02 PM, Heiko Stübner <heiko@sntech.de> wrote:
> > Hi Jay,
> >
> > Am Donnerstag, 21. April 2016, 11:58:12 schrieb Jianqun Xu:
> >> This patch adds rk3399.dtsi for rk3399 found on Rockchip
> >> RK3399 SoCs, also add rk3399-evb.dts for Rockchip RK3399
> >> Evaluation Board.
> >>
> >> Patch is tested on RK3399 evb.
> >>
> >> Signed-off-by: Jianqun Xu <jay.xu@rock-chips.com>
> >
> > please split this into
> > - patch adding the dtsi
> > - patch adding the evb dts
> > - patch adding the new board to bindings/arm/rockchip.txt
> >
> > more inline below
>
> Also don't forget to remove the controversial pmu bits for now (as
> discussed earlier) so this can land while all those kinks are being
> worked out.
>
> >> + sdhci: sdhci@fe330000 {
> >> + compatible = "arasan,sdhci-5.1";
> >
> > not 100% sure, but we might want a
> >
> > compatible = "rockchip,rk3399-sdhci-5.1",
> > "arasan,sdhci-5.1";
> >
> > allowing us to get more specific, if implementation oddities surface
> > later.
>
> I agree with Heiko. This sounds very sane to me, too, and matches
> previous discussions.
>
> >> + reg = <0x0 0xfe330000 0x0 0x10000>;
> >> + interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> >> + clocks = <&cru SCLK_EMMC>, <&cru ACLK_EMMC>;
> >> + clock-names = "clk_xin", "clk_ahb";
> >> + phys = <&emmc_phy>;
> >> + phy-names = "phy_arasan";
> >> + status = "disabled";
> >> + };
> >> +
> >> + usb2phy: usb2phy {
> >> + compatible = "rockchip,rk3399-usb-phy";
> >
> > this doesn't look like it got submitted yet.
> >
> > Also, the newer socs (rk3399. rk3036, rk3228) seem to use a different
> > usbphy block than rk3288 and before (with a big bunch of new phy-related
> > register blocks I haven't looked at yet) - so this should probably get a
> > new driver as well and not be crammed into the current phy driver, which
> > is for the older picophy (or what it was called).
> >
> >> + rockchip,grf = <&grf>;
> >> + #address-cells = <1>;
> >> + #size-cells = <0>;
> >> +
> >> + usb2phy0: usb2-phy0 {
> >> + #phy-cells = <0>;
> >> + #clock-cells = <0>;
> >> + reg = <0xe458>;
> >> + };
> >
> > When we're doing a new driver, could we please get rid of these subnodes
> > and instead access phys via something like
> >
> > phys = <&usb2phy 0>;
>
> From what I recall during the submission of the previous PHY Kishon
> preferred the subnodes. I think I made a fool of myself in the last
> discussion about this because I reported bugs in my downstream kernel
> that didn't exist upstream, but if you want to read it you can see
> here:
>
> https://patchwork.kernel.org/patch/5474871/
>
> I believe patch v6 used IDs like Heiko is suggesting and it turned to
> subnodes in v7 based on Kishon's request. Since PHY code and bindings
> are Kishon's call, I have a feeling his opinion will trump here.
After Doug pointed me to that old discussion, I tend to agree - aka use sub-
nodes.
> >> +
> >> + usb2phy1: usb2-phy1 {
> >> + #phy-cells = <0>;
> >> + #clock-cells = <0>;
> >> + reg = <0xe468>;
> >> + };
> >> + };
> >> +
> >> + usb_host0_echi: usb@fe380000 {
> >
> > not "echi" please :-)
>
> Just because it took me an extra reading to understand, he means turn
> "echi" to "ehci".
>
> >> + compatible = "generic-ehci";
> >> + reg = <0x0 0xfe380000 0x0 0x20000>;
> >> + interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
> >> + clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>;
> >> + clock-names = "hclk_host0", "hclk_host0_arb";
> >> + phys = <&usb2phy0>;
> >> + phy-names = "usb2_phy0";
> >> + status = "disabled";
> >> + };
> >
> > [...]
> >
> >> + usbdrd3_0: usb@fe800000 {
> >> + compatible = "rockchip,dwc3";
> >
> > is this in some tree already?
>
> I'm really surprised that there's not some generic fallback for
> "dwc3-of-simple.c". I would have expected:
> "rockchip,rk3399-dwc3", "synopsis,dwc3";
>
> ...but that doesn't appear to be in the bindings. Weird.
>
> >> + i2c1: i2c@ff110000 {
> >> + compatible = "rockchip,rk3399-i2c";
> >
> > David respun the rk3399 i2c-support on tuesday, so this and the others
> > below are waiting on Wolfram to take a look.
>
> I think it can work with the rk3288-i2c as a fallback, at least for
> low speed stuff, right? Should this be:
>
> compatible = "rockchip,rk3399-i2c", "rockchip,rk3288-i2c"
>
> Looks like that was done for rk3368.
The rk3368 has virtually the same ip blocks as the rk3288, so the i2c
controllers actually are the same. Not sure how true this is for the rk3399
though.
[toc] | [prev] | [next] | [standalone]
| From | "Huang, Tao" <huangtao@rock-chips.com> |
|---|---|
| Date | 2016-04-22 06:30 +0200 |
| Message-ID | <rqAT8-4e7-29@gated-at.bofh.it> |
| In reply to | #1384617 |
Hi, Doug:
On 2016年04月22日 06:38, Doug Anderson wrote:
>
>>> + i2c1: i2c@ff110000 {
>>> + compatible = "rockchip,rk3399-i2c";
>> David respun the rk3399 i2c-support on tuesday, so this and the others below
>> are waiting on Wolfram to take a look.
> I think it can work with the rk3288-i2c as a fallback, at least for
> low speed stuff, right? Should this be:
>
> compatible = "rockchip,rk3399-i2c", "rockchip,rk3288-i2c"
>
> Looks like that was done for rk3368.
No. For RK3399 I2C controller:
The I2C controller uses the APB clock/clk_i2c as the working clock. The
APB clock will determine the I2C bus clock, clk_i2c is the function clk,
up to 200MHz.
Chips such as RK3288/3368 only uses APB clock. So old driver do not work
on RK3399.
Thanks,
Huang, Tao
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web