* Re: [PATCH v1 27/50] ARM: dts: exynos: align bus_wcore OPPs in Exynos5420
From: Lukasz Luba @ 2019-07-17 16:58 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: devicetree, linux-kernel, linux-arm-kernel,
linux-samsung-soc@vger.kernel.org, linux-clk, mturquette, sboyd,
Bartłomiej Żołnierkiewicz, kgene, mark.rutland,
robh+dt, Chanwoo Choi, kyungmin.park, Andrzej Hajda,
Marek Szyprowski, s.nawrocki, myungjoo.ham
In-Reply-To: <CAJKOXPfFZL8q9hM1vPsLq+Qxe-gMz4c8j0jgFKfdf5qs68MTmA@mail.gmail.com>
Hi Krzysztof,
On 7/17/19 12:15 PM, Krzysztof Kozlowski wrote:
> On Mon, 15 Jul 2019 at 14:44, Lukasz Luba <l.luba@partner.samsung.com> wrote:
>>
>> This is the most important bus in the Exynos5x SoC. The whole communication
>> inside SoC does through that bus (apart from direct requests from CCI to
>> DRAM controller). It is also modeled as a master bus in devfreq framework.
>> It is also the only one OPP table throughout other buses which has voltage
>> values. The devfreq software controls the speed of that bus and other
>> buses. The other buses follows the rate of the master. There is only one
>> regulator. The old lowest OPP had pair 925mV, 84MHz which is enough for
>
> s/lowest/slowest/
please see below
>
>> this frequency. However, due to the fact that the other buses follows the
>> WCORE bus by taking the OPP from their table with the same id, e.g. opp02,
>> the children frequency should be stable with the set voltage.
>> It could cause random faults very hard to debug.
>> Thus, the patch removes the lowest OPP to make other buses' lowest OPPs
>
> s/lowest/slowest/
Actually, I have double checked that, because we always used this
terminology: low OPP, high OPP, lower OPPs, higher OPPs. I can change
it here for you, but I think this is not something that people are used
to. Please check EAS pdf documentation or this file:
https://www.kernel.org/doc/Documentation/scheduler/sched-energy.txt
i.e. "running at a lower OPP" or "high OPPs", "lowest OPPs".
Regards,
Lukasz
>
>> working. The new lowest OPP has voltage high enough for buses working up
>> to 333MHz. It also changes the frequencies of the OPPs to align them to
>> PLL value such that it is possible to set them using only a divider without
>> reprogramming OPP.
>
> Reprogramming OPP? What is it?
>
>> Reprogramming the PLL was not set, so the real frequency
>
> I understood from the previous that reprogramming the OPP (PLL?) was
> happening... Please rephrase entire sentence.
>
> BR,
> Krzysztof
>
>> values were not the one from the OPP table, which could confuse the
>> governor algorithms which relay on OPP speed values making the system to
>> behave weird.
>>
>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>> ---
>> arch/arm/boot/dts/exynos5420.dtsi | 12 ++++--------
>> 1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
>> index f8c36ff0d4c3..a355c76af5a5 100644
>> --- a/arch/arm/boot/dts/exynos5420.dtsi
>> +++ b/arch/arm/boot/dts/exynos5420.dtsi
>> @@ -1107,22 +1107,18 @@
>> compatible = "operating-points-v2";
>>
>> opp00 {
>> - opp-hz = /bits/ 64 <84000000>;
>> - opp-microvolt = <925000>;
>> + opp-hz = /bits/ 64 <150000000>;
>> + opp-microvolt = <950000>;
>> };
>> opp01 {
>> - opp-hz = /bits/ 64 <111000000>;
>> + opp-hz = /bits/ 64 <200000000>;
>> opp-microvolt = <950000>;
>> };
>> opp02 {
>> - opp-hz = /bits/ 64 <222000000>;
>> + opp-hz = /bits/ 64 <300000000>;
>> opp-microvolt = <950000>;
>> };
>> opp03 {
>> - opp-hz = /bits/ 64 <333000000>;
>> - opp-microvolt = <950000>;
>> - };
>> - opp04 {
>> opp-hz = /bits/ 64 <400000000>;
>> opp-microvolt = <987500>;
>> };
>> --
>> 2.17.1
>>
>
>
^ permalink raw reply
* Re: [PATCHv8 5/5] coresight: cpu-debug: Add support for Qualcomm Kryo
From: Mathieu Poirier @ 2019-07-17 16:56 UTC (permalink / raw)
To: Sai Prakash Ranjan
Cc: Greg Kroah-Hartman, Suzuki K Poulose, Leo Yan, Alexander Shishkin,
Mike Leach, Rob Herring, Bjorn Andersson, devicetree, David Brown,
Mark Rutland, Rajendra Nayak, Vivek Gautam, Sibi Sankar,
linux-arm-kernel, linux-kernel, linux-arm-msm, Marc Gonzalez
In-Reply-To: <e2c4cc7c6ccaa5695f25af20c8e487ac53b39955.1562940244.git.saiprakash.ranjan@codeaurora.org>
On Fri, Jul 12, 2019 at 07:46:27PM +0530, Sai Prakash Ranjan wrote:
> Add support for coresight CPU debug module on Qualcomm
> Kryo CPUs. This patch adds the UCI entries for Kryo CPUs
> found on MSM8996 which shares the same PIDs as ETMs.
>
> Without this, below error is observed on MSM8996:
>
> [ 5.429867] OF: graph: no port node found in /soc/debug@3810000
> [ 5.429938] coresight-etm4x: probe of 3810000.debug failed with error -22
> [ 5.435415] coresight-cpu-debug 3810000.debug: Coresight debug-CPU0 initialized
> [ 5.446474] OF: graph: no port node found in /soc/debug@3910000
> [ 5.448927] coresight-etm4x: probe of 3910000.debug failed with error -22
> [ 5.454681] coresight-cpu-debug 3910000.debug: Coresight debug-CPU1 initialized
> [ 5.487765] OF: graph: no port node found in /soc/debug@3a10000
> [ 5.488007] coresight-etm4x: probe of 3a10000.debug failed with error -22
> [ 5.493024] coresight-cpu-debug 3a10000.debug: Coresight debug-CPU2 initialized
> [ 5.501802] OF: graph: no port node found in /soc/debug@3b10000
> [ 5.512901] coresight-etm4x: probe of 3b10000.debug failed with error -22
> [ 5.513192] coresight-cpu-debug 3b10000.debug: Coresight debug-CPU3 initialized
>
> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
> ---
> .../hwtracing/coresight/coresight-cpu-debug.c | 33 +++++++++----------
> drivers/hwtracing/coresight/coresight-priv.h | 10 +++---
> 2 files changed, 21 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> index 2463aa7ab4f6..96544b348c27 100644
> --- a/drivers/hwtracing/coresight/coresight-cpu-debug.c
> +++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> @@ -646,24 +646,23 @@ static int debug_remove(struct amba_device *adev)
> return 0;
> }
>
> +static const struct amba_cs_uci_id uci_id_debug[] = {
> + {
> + /* CPU Debug UCI data */
> + .devarch = 0x47706a15,
> + .devarch_mask = 0xfff0ffff,
> + .devtype = 0x00000015,
> + }
> +};
> +
> static const struct amba_id debug_ids[] = {
> - { /* Debug for Cortex-A53 */
> - .id = 0x000bbd03,
> - .mask = 0x000fffff,
> - },
> - { /* Debug for Cortex-A57 */
> - .id = 0x000bbd07,
> - .mask = 0x000fffff,
> - },
> - { /* Debug for Cortex-A72 */
> - .id = 0x000bbd08,
> - .mask = 0x000fffff,
> - },
> - { /* Debug for Cortex-A73 */
> - .id = 0x000bbd09,
> - .mask = 0x000fffff,
> - },
> - { 0, 0 },
> + CS_AMBA_ID(0x000bbd03), /* Cortex-A53 */
> + CS_AMBA_ID(0x000bbd07), /* Cortex-A57 */
> + CS_AMBA_ID(0x000bbd08), /* Cortex-A72 */
> + CS_AMBA_ID(0x000bbd09), /* Cortex-A73 */
> + CS_AMBA_UCI_ID(0x000f0205, uci_id_debug), /* Qualcomm Kryo */
> + CS_AMBA_UCI_ID(0x000f0211, uci_id_debug), /* Qualcomm Kryo */
> + {},
> };
>
> static struct amba_driver debug_driver = {
> diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
> index 7d401790dd7e..41ae5863104d 100644
> --- a/drivers/hwtracing/coresight/coresight-priv.h
> +++ b/drivers/hwtracing/coresight/coresight-priv.h
> @@ -185,11 +185,11 @@ static inline int etm_writel_cp14(u32 off, u32 val) { return 0; }
> }
>
> /* coresight AMBA ID, full UCI structure: id table entry. */
> -#define CS_AMBA_UCI_ID(pid, uci_ptr) \
> - { \
> - .id = pid, \
> - .mask = 0x000fffff, \
> - .data = uci_ptr \
> +#define CS_AMBA_UCI_ID(pid, uci_ptr) \
> + { \
> + .id = pid, \
> + .mask = 0x000fffff, \
> + .data = (void *)uci_ptr \
> }
I will pickup this patch - it will show up in my next tree when rc1 comes out.
Thanks,
Mathieu
>
> /* extract the data value from a UCI structure given amba_id pointer. */
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
>
^ permalink raw reply
* [PATCH] MIPS: JZ4740: DTS: Add I2C nodes
From: Alexandre GRIVEAUX @ 2019-07-17 16:50 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Ralf Baechle, Paul Burton, James Hogan,
Alexandre GRIVEAUX
Cc: linux-mips, linux-kernel, devicetree
Add the devicetree nodes for the I2C core of the JZ4780 SoC, disabled
by default.
---
arch/mips/boot/dts/ingenic/jz4780.dtsi | 86 ++++++++++++++++++++++++++
1 file changed, 86 insertions(+)
diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index b03cdec56de9..a76ecd69bfd0 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -239,6 +239,92 @@
status = "disabled";
};
+ i2c0: i2c@10050000 {
+ compatible = "ingenic,jz4780-i2c";
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ reg = <0x10050000 0x1000>;
+
+ interrupt-parent = <&intc>;
+ interrupts = <60>;
+
+ clocks = <&cgu JZ4780_CLK_SMB0>;
+ clock-frequency = <100000>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pins_i2c0_data>;
+
+ status = "disabled";
+ };
+
+ i2c1: i2c@10051000 {
+ compatible = "ingenic,jz4780-i2c";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x10051000 0x1000>;
+
+ interrupt-parent = <&intc>;
+ interrupts = <59>;
+
+ clocks = <&cgu JZ4780_CLK_SMB1>;
+ clock-frequency = <100000>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pins_i2c1_data>;
+
+ status = "disabled";
+ };
+
+ i2c2: i2c@10052000 {
+ compatible = "ingenic,jz4780-i2c";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x10052000 0x1000>;
+
+ interrupt-parent = <&intc>;
+ interrupts = <58>;
+
+ clocks = <&cgu JZ4780_CLK_SMB2>;
+ clock-frequency = <100000>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pins_i2c2_data>;
+
+ status = "disabled";
+ };
+
+ i2c3: i2c@10053000 {
+ compatible = "ingenic,jz4780-i2c";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x10053000 0x1000>;
+
+ interrupt-parent = <&intc>;
+ interrupts = <57>;
+
+ clocks = <&cgu JZ4780_CLK_SMB3>;
+ clock-frequency = <100000>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pins_i2c3_data>;
+
+ status = "disabled";
+ };
+
+ i2c4: i2c@10054000 {
+ compatible = "ingenic,jz4780-i2c";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x10054000 0x1000>;
+
+ interrupt-parent = <&intc>;
+ interrupts = <56>;
+
+ clocks = <&cgu JZ4780_CLK_SMB4>;
+ clock-frequency = <100000>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pins_i2c4_data>;
+
+ status = "disabled";
+ };
+
watchdog: watchdog@10002000 {
compatible = "ingenic,jz4780-watchdog";
reg = <0x10002000 0x10>;
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v1 1/1] arm64: dts: rockchip: Add support for TB-96AI board
From: Manivannan Sadhasivam @ 2019-07-17 15:49 UTC (permalink / raw)
To: Elon Zhang
Cc: heiko, mark.rutland, robh+dt, linux-rockchip, linux-arm-kernel,
linux-kernel, devicetree
In-Reply-To: <20190717154752.GA13269@Mani-XPS-13-9360>
On Wed, Jul 17, 2019 at 09:17:52PM +0530, Manivannan Sadhasivam wrote:
> Hi Elon,
>
> Thanks for the patch. Overall, this patch needs a bit of cleanup. There
> are many nodes added which are not available in mainline.
>
> Please see comments inline.
>
> On Thu, Jul 11, 2019 at 10:12:09AM +0800, Elon Zhang wrote:
> > Add devicetree support for RK3399Pro TB-96AI board, one of
> > the 96Boards family.
> >
> > The TB-96AI board is a 96Boards Compute SOM design, launched
> > by Linaro, Rockchip and Beiqicloud.
> >
> > More information can be obtained from the following websites:
> > 1.https://www.96boards.org/product/tb-96ai/
> > 2.http://t.rock-chips.com/
> > 3.http://www.beiqicloud.com/
> >
> > This patch add basic node for the board and support booting up
> > to Fedora.
> >
> > Signed-off-by: Elon Zhang <zhangzj@rock-chips.com>
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
> > index 5f2687acbf94..3d6c8d4363b5 100644
> > --- a/arch/arm64/boot/dts/rockchip/Makefile
> > +++ b/arch/arm64/boot/dts/rockchip/Makefile
> > @@ -27,3 +27,4 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-rock960.dtb
> > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-rockpro64.dtb
> > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-sapphire.dtb
> > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-sapphire-excavator.dtb
> > +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399pro-tb-96ai.dtb
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3399pro-tb-96ai.dts b/arch/arm64/boot/dts/rockchip/rk3399pro-tb-96ai.dts
> > new file mode 100644
> > index 000000000000..1935df99065d
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/rockchip/rk3399pro-tb-96ai.dts
> > @@ -0,0 +1,629 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > +/*
> > + * Copyright (c) 2019 Fuzhou Rockchip Electronics Co., Ltd.
> > + */
> > +
> > +/dts-v1/;
> > +#include "rk3399pro.dtsi"
> > +#include "rk3399-opp.dtsi"
> > +
> > +/ {
> > + compatible = "rockchip,rk3399pro-tb-96ai", "rockchip,rk3399pro";
> > +
>
> I think the manufacturer of this board is, Xiamen Beiqi Technology Co. Ltd.
> So, the compatible should be:
>
> compatible = "beiqui,rk3399pro-tb-96ai", "rockchip,rk3399pro";
s/beiqui/beiqi
>
> And there should a separate patch to add the vendor prefix. You can refer
> below patch:
>
> https://lkml.org/lkml/2019/7/17/39
>
> > + chosen {
> > + stdout-path = "serial2:1500000n8";
> > + };
> > +
> > + xin32k: xin32k {
> > + compatible = "fixed-clock";
> > + clock-frequency = <32768>;
> > + clock-output-names = "xin32k";
> > + #clock-cells = <0>;
> > + };
> > +
> > + vcc_phy: vcc-phy-regulator {
> > + compatible = "regulator-fixed";
> > + regulator-name = "vcc_phy";
> > + regulator-always-on;
> > + regulator-boot-on;
> > + };
> > +
> > + vcc5v0_sys: vccsys {
> > + compatible = "regulator-fixed";
> > + regulator-name = "vcc5v0_sys";
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <5000000>;
> > + regulator-max-microvolt = <5000000>;
> > + };
> > +
> > + vdd_log: vdd_log {
> > + compatible = "regulator-fixed";
> > + regulator-name = "vdd_log";
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <900000>;
> > + regulator-max-microvolt = <900000>;
> > + };
> > +
> > + leds: gpio-leds {
> > + compatible = "gpio-leds";
> > + pinctrl-names = "default";
> > + pinctrl-0 =<&leds_gpio>;
>
> Leave a space after =
>
> > +
> > + led@1 {
> > + gpios = <&gpio2 5 GPIO_ACTIVE_HIGH>;
> > + label = "system_work_led1";
> > + retain-state-suspended;
> > + };
> > +
>
> We are using a standard LED formats for all 96Boards. Please see,
> rk3399-rock960.dts for reference. Since there is only user leds
> (apart from power leds), just define those as per the format.
>
> > + led@2 {
> > + gpios = <&gpio2 4 GPIO_ACTIVE_HIGH>;
> > + label = "system_work_led2";
> > + retain-state-suspended;
> > + };
> > +
> > + led@3 {
> > + gpios = <&gpio2 3 GPIO_ACTIVE_HIGH>;
> > + label = "system_work_led3";
> > + retain-state-suspended;
> > + };
> > + };
> > +};
> > +
> > +&cpu_l0 {
> > + cpu-supply = <&vdd_cpu_l>;
> > +};
> > +
> > +&cpu_l1 {
> > + cpu-supply = <&vdd_cpu_l>;
> > +};
> > +
> > +&cpu_l2 {
> > + cpu-supply = <&vdd_cpu_l>;
> > +};
> > +
> > +&cpu_l3 {
> > + cpu-supply = <&vdd_cpu_l>;
> > +};
> > +
> > +&cpu_b0 {
> > + cpu-supply = <&vdd_cpu_b>;
> > +};
> > +
> > +&cpu_b1 {
> > + cpu-supply = <&vdd_cpu_b>;
> > +};
> > +
> > +&emmc_phy {
> > + status = "okay";
> > +};
> > +
> > +&i2c0 {
> > + status = "okay";
> > + i2c-scl-rising-time-ns = <180>;
> > + i2c-scl-falling-time-ns = <30>;
> > + clock-frequency = <400000>;
> > +
> > + rk809: pmic@20 {
> > + compatible = "rockchip,rk809";
>
> It looks like this driver is not present in mainline yet. I can see
> some old patches in lkml archive but not sure about the status of those.
> So, please remove this node and use dummy regulators where applicable.
>
> > + reg = <0x20>;
> > + interrupt-parent = <&gpio1>;
> > + interrupts = <RK_PC2 IRQ_TYPE_LEVEL_LOW>;
> > + pinctrl-names = "default", "pmic-sleep",
> > + "pmic-power-off", "pmic-reset";
> > + pinctrl-0 = <&pmic_int_l>;
> > + pinctrl-1 = <&soc_slppin_slp>, <&rk809_slppin_slp>;
> > + pinctrl-2 = <&soc_slppin_gpio>, <&rk809_slppin_pwrdn>;
> > + pinctrl-3 = <&soc_slppin_gpio>,<&rk809_slppin_null>;
> > + rockchip,system-power-controller;
> > + pmic-reset-func = <1>;
> > + wakeup-source;
> > + #clock-cells = <1>;
> > + clock-output-names = "rk808-clkout1", "rk808-clkout2";
> > +
> > + vcc1-supply = <&vcc5v0_sys>;
> > + vcc2-supply = <&vcc5v0_sys>;
> > + vcc3-supply = <&vcc5v0_sys>;
> > + vcc4-supply = <&vcc5v0_sys>;
> > + vcc5-supply = <&vcc_buck5>;
> > + vcc6-supply = <&vcc_buck5>;
> > + vcc7-supply = <&vcc3v3_sys>;
> > + vcc8-supply = <&vcc3v3_sys>;
> > + vcc9-supply = <&vcc5v0_sys>;
> > +
> > + pwrkey {
> > + status = "okay";
> > + };
> > +
> > + rtc {
> > + status = "okay";
> > + };
> > +
> > + pinctrl_rk8xx: pinctrl_rk8xx {
> > + gpio-controller;
> > + #gpio-cells = <2>;
> > +
> > + rk809_slppin_null: rk809_slppin_null {
> > + pins = "gpio_slp";
> > + function = "pin_fun0";
> > + };
> > +
> > + rk809_slppin_slp: rk809_slppin_slp {
> > + pins = "gpio_slp";
> > + function = "pin_fun1";
> > + };
> > +
> > + rk809_slppin_pwrdn: rk809_slppin_pwrdn {
> > + pins = "gpio_slp";
> > + function = "pin_fun2";
> > + };
> > +
> > + rk809_slppin_rst: rk809_slppin_rst {
> > + pins = "gpio_slp";
> > + function = "pin_fun3";
> > + };
> > + };
> > +
> > + regulators {
> > + vdd_center: DCDC_REG1 {
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <750000>;
> > + regulator-max-microvolt = <1350000>;
> > + regulator-initial-mode = <0x2>;
> > + regulator-name = "vdd_center";
> > + regulator-state-mem {
> > + regulator-on-in-suspend;
> > + regulator-suspend-microvolt = <900000>;
> > + };
> > + };
> > +
> > + vdd_cpu_l: DCDC_REG2 {
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <750000>;
> > + regulator-max-microvolt = <1350000>;
> > + regulator-ramp-delay = <6001>;
> > + regulator-initial-mode = <0x2>;
> > + regulator-name = "vdd_cpu_l";
> > + regulator-state-mem {
> > + regulator-off-in-suspend;
> > + };
> > + };
> > +
> > + vcc_ddr: DCDC_REG3 {
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-name = "vcc_ddr";
> > + regulator-initial-mode = <0x2>;
> > + regulator-state-mem {
> > + regulator-on-in-suspend;
> > + };
> > + };
> > +
> > + vcc3v3_sys: DCDC_REG4 {
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <3300000>;
> > + regulator-max-microvolt = <3300000>;
> > + regulator-initial-mode = <0x2>;
> > + regulator-name = "vcc3v3_sys";
> > + regulator-state-mem {
> > + regulator-on-in-suspend;
> > + regulator-suspend-microvolt = <3300000>;
> > + };
> > + };
> > +
> > + vcc_buck5: DCDC_REG5 {
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <2200000>;
> > + regulator-max-microvolt = <2200000>;
> > + regulator-name = "vcc_buck5";
> > + regulator-state-mem {
> > + regulator-on-in-suspend;
> > + regulator-suspend-microvolt = <2200000>;
> > + };
> > + };
> > +
> > + vcca_0v9: LDO_REG1 {
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <900000>;
> > + regulator-max-microvolt = <900000>;
> > + regulator-name = "vcca_0v9";
> > + regulator-state-mem {
> > + regulator-off-in-suspend;
> > + };
> > + };
> > +
> > + vcc_1v8: LDO_REG2 {
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <1800000>;
> > + regulator-max-microvolt = <1800000>;
> > +
> > + regulator-name = "vcc_1v8";
> > + regulator-state-mem {
> > + regulator-on-in-suspend;
> > + regulator-suspend-microvolt = <1800000>;
> > + };
> > + };
> > +
> > + vcc0v9_soc: LDO_REG3 {
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <900000>;
> > + regulator-max-microvolt = <900000>;
> > +
> > + regulator-name = "vcc0v9_soc";
> > + regulator-state-mem {
> > + regulator-on-in-suspend;
> > + regulator-suspend-microvolt = <900000>;
> > + };
> > + };
> > +
> > + vcca_1v8: LDO_REG4 {
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <1800000>;
> > + regulator-max-microvolt = <1800000>;
> > +
> > + regulator-name = "vcca_1v8";
> > + regulator-state-mem {
> > + regulator-off-in-suspend;
> > + };
> > + };
> > +
> > + vdd1v5_dvp: LDO_REG5 {
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <1500000>;
> > + regulator-max-microvolt = <1500000>;
> > +
> > + regulator-name = "vdd1v5_dvp";
> > + regulator-state-mem {
> > + regulator-off-in-suspend;
> > + };
> > + };
> > +
> > + vcc_1v5: LDO_REG6 {
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <1500000>;
> > + regulator-max-microvolt = <1500000>;
> > +
> > + regulator-name = "vcc_1v5";
> > + regulator-state-mem {
> > + regulator-off-in-suspend;
> > + };
> > + };
> > +
> > + vcc_3v0: LDO_REG7 {
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <3000000>;
> > + regulator-max-microvolt = <3000000>;
> > +
> > + regulator-name = "vcc_3v0";
> > + regulator-state-mem {
> > + regulator-off-in-suspend;
> > + };
> > + };
> > +
> > + vccio_sd: LDO_REG8 {
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <1800000>;
> > + regulator-max-microvolt = <3300000>;
> > +
> > + regulator-name = "vccio_sd";
> > + regulator-state-mem {
> > + regulator-on-in-suspend;
> > + regulator-suspend-microvolt = <3300000>;
> > + };
> > + };
> > +
> > + vcc_sd: LDO_REG9 {
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <3300000>;
> > + regulator-max-microvolt = <3300000>;
> > +
> > + regulator-name = "vcc_sd";
> > + regulator-state-mem {
> > + regulator-on-in-suspend;
> > + regulator-suspend-microvolt = <3300000>;
> > + };
> > + };
> > +
> > + vcc5v0_usb: SWITCH_REG1 {
> > + regulator-min-microvolt = <5000000>;
> > + regulator-max-microvolt = <5000000>;
> > +
> > + regulator-name = "vcc5v0_usb";
> > + regulator-state-mem {
> > + regulator-off-in-suspend;
> > + };
> > + };
> > +
> > + vccio_3v3: SWITCH_REG2 {
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <3300000>;
> > + regulator-max-microvolt = <3300000>;
> > +
> > + regulator-name = "vccio_3v3";
> > + regulator-state-mem {
> > + regulator-off-in-suspend;
> > + };
> > + };
> > + };
> > + };
> > +
> > + vdd_cpu_b: tcs452x@1c {
> > + compatible = "tcs,tcs452x";
>
> Again, there is no driver for this.
>
> > + reg = <0x1c>;
> > + vin-supply = <&vcc5v0_sys>;
> > + regulator-compatible = "fan53555-reg";
> > + pinctrl-0 = <&vsel1_gpio>;
> > + vsel-gpios = <&gpio1 RK_PC1 GPIO_ACTIVE_HIGH>;
> > + regulator-name = "vdd_cpu_b";
> > + regulator-min-microvolt = <712500>;
> > + regulator-max-microvolt = <1500000>;
> > + regulator-ramp-delay = <2300>;
> > + fcs,suspend-voltage-selector = <1>;
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-initial-state = <3>;
> > + regulator-state-mem {
> > + regulator-off-in-suspend;
> > + };
> > + };
> > +
> > + vdd_gpu: tcs452x@10 {
> > + compatible = "tcs,tcs452x";
>
> ditto.
>
> > + status = "okay";
> > + reg = <0x10>;
> > + vin-supply = <&vcc5v0_sys>;
> > + regulator-compatible = "fan53555-reg";
> > + pinctrl-0 = <&vsel2_gpio>;
> > + vsel-gpios = <&gpio1 RK_PB6 GPIO_ACTIVE_HIGH>;
> > + regulator-name = "vdd_gpu";
> > + regulator-min-microvolt = <735000>;
> > + regulator-max-microvolt = <1400000>;
> > + regulator-ramp-delay = <2300>;
> > + fcs,suspend-voltage-selector = <1>;
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-state-mem {
> > + regulator-off-in-suspend;
> > + };
> > + };
> > +
> > +};
> > +
> > +&i2c8 {
> > + status = "okay";
> > + i2c-scl-rising-time-ns = <345>;
> > + i2c-scl-falling-time-ns = <11>;
> > + clock-frequency = <100000>;
> > +
> > + fusb0: fusb30x@22 {
> > + compatible = "fairchild,fusb302";
>
> ditto, please remove.
>
> > + reg = <0x22>;
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&fusb0_int>;
> > + int-n-gpios = <&gpio1 RK_PA2 GPIO_ACTIVE_HIGH>;
> > + vbus-5v-gpios = <&gpio0 RK_PA1 GPIO_ACTIVE_LOW>;
> > + status = "okay";
> > + };
> > +
> > +};
> > +
> > +&io_domains {
> > + status = "okay";
> > + bt656-supply = <&vcca_1v8>; /* APIO2_VDD */
> > + audio-supply = <&vcca_1v8>; /* APIO5_VDD */
> > + sdmmc-supply = <&vccio_sd>; /* SDMMC0_VDD */
> > + gpio1830-supply = <&vcc_1v8>; /* APIO4_VDD */
> > +};
> > +
> > +&pinctrl {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&npu_ref_clk>;
> > +
> > + fusb30x {
> > + fusb0_int: fusb0-int {
> > + rockchip,pins =
> > + <1 RK_PA2 0 &pcfg_pull_up>;
> > + };
> > + };
>
> not needed.
>
> > +
> > + gpio-leds {
> > + leds_gpio: leds-gpio {
> > + rockchip,pins =
> > + <2 5 RK_FUNC_GPIO &pcfg_pull_up>,
> > + <2 4 RK_FUNC_GPIO &pcfg_pull_up>,
> > + <2 3 RK_FUNC_GPIO &pcfg_pull_up>;
>
> I don't think we need pull-up here.
>
> > + };
> > + };
> > +
> > + npu_clk {
> > + npu_ref_clk: npu-ref-clk {
> > + rockchip,pins =
> > + <0 RK_PA2 1 &pcfg_pull_none>;
> > + };
> > + };
> > +
> > + pmic {
> > + pmic_int_l: pmic-int-l {
> > + rockchip,pins =
> > + <1 RK_PC2 0 &pcfg_pull_up>;
> > + };
> > +
> > + soc_slppin_gpio: soc-slppin-gpio {
> > + rockchip,pins =
> > + <1 RK_PA5 0 &pcfg_output_low>;
> > + };
> > +
> > + soc_slppin_slp: soc-slppin-slp {
> > + rockchip,pins =
> > + <1 RK_PA5 1 &pcfg_pull_down>;
> > + };
> > +
> > + vsel1_gpio: vsel1-gpio {
> > + rockchip,pins =
> > + <1 RK_PC1 0 &pcfg_pull_down>;
> > + };
> > +
> > + vsel2_gpio: vsel2-gpio {
> > + rockchip,pins =
> > + <1 RK_PB6 0 &pcfg_pull_down>;
> > + };
> > + };
>
> not needed.
>
> > +
> > + usb3 {
> > + usb3_host_en: usb3-host-en {
> > + rockchip,pins =
> > + <2 RK_PA2 RK_FUNC_GPIO &pcfg_output_high>;
> > + };
> > + };
> > +};
> > +
> > +&pmu_io_domains {
> > + status = "okay";
> > + pmu1830-supply = <&vcc_1v8>;
> > +};
> > +
> > +&pwm0 {
> > + status = "okay";
> > +};
> > +
> > +&pwm2 {
> > + status = "okay";
> > +};
> > +
> > +&saradc {
> > + status = "okay";
> > + vref-supply = <&vcc_1v8>;
> > +};
> > +
> > +&sdhci {
> > + bus-width = <8>;
> > + mmc-hs400-1_8v;
> > + supports-emmc;
>
> there is no such property, so please remove. Since this controller is
> used for emmc, you can use "no-sd" and "no-sdio" properties if needed.
>
> > + non-removable;
> > + keep-power-in-suspend;
> > + mmc-hs400-enhanced-strobe;
> > + status = "okay";
> > +};
> > +
> > +&sdmmc {
> > + clock-frequency = <150000000>;
> > + clock-freq-min-max = <400000 150000000>;
> > + supports-sd;
>
> Same as above, property not available. Use, "no-sdio" and "no-emmc" if
> needed.
>
> > + bus-width = <4>;
> > + cap-mmc-highspeed;
> > + cap-sd-highspeed;
> > + disable-wp;
> > + num-slots = <1>;
>
> not needed.
>
> > + vqmmc-supply = <&vccio_sd>;
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_cd &sdmmc_bus4>;
> > + status = "okay";
> > +};
> > +
> > +&tcphy0 {
> > + extcon = <&fusb0>;
> > + status = "okay";
> > +};
> > +
> > +&tcphy1 {
> > + status = "okay";
> > +};
> > +
> > +&tsadc {
> > + rockchip,hw-tshut-mode = <1>; /* tshut mode 0:CRU 1:GPIO */
> > + rockchip,hw-tshut-polarity = <1>; /* tshut polarity 0:LOW 1:HIGH */
>
> Any clue about shutdown temperature? Not necessary now but good to have.
>
> Thanks,
> Mani
> > + status = "okay";
> > +};
> > +
> > +&u2phy0 {
> > + status = "okay";
> > + extcon = <&fusb0>;
> > +
> > + u2phy0_otg: otg-port {
> > + status = "okay";
> > + };
> > +
> > + u2phy0_host: host-port {
> > + phy-supply = <&vcc5v0_usb>;
> > + status = "okay";
> > + };
> > +};
> > +
> > +&u2phy1 {
> > + status = "okay";
> > +
> > + u2phy1_otg: otg-port {
> > + status = "okay";
> > + };
> > +
> > + u2phy1_host: host-port {
> > + phy-supply = <&vcc5v0_usb>;
> > + status = "okay";
> > + };
> > +};
> > +
> > +&uart0 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&uart0_xfer &uart0_cts>;
> > + status = "okay";
> > +};
> > +
> > +&uart2 {
> > + status = "okay";
> > +};
> > +
> > +&uart4 {
> > + status = "okay";
> > +};
> > +
> > +&usb_host0_ehci {
> > + status = "okay";
> > +};
> > +
> > +&usb_host1_ehci {
> > + status = "okay";
> > +};
> > +
> > +&usb_host0_ohci {
> > + status = "okay";
> > +};
> > +
> > +&usb_host1_ohci {
> > + status = "okay";
> > +};
> > +
> > +&usbdrd3_0 {
> > + extcon = <&fusb0>;
> > + status = "okay";
> > +};
> > +
> > +&usbdrd3_1 {
> > + status = "okay";
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&usb3_host_en>;
> > +};
> > +
> > +&usbdrd_dwc3_0 {
> > + status = "okay";
> > +};
> > +
> > +&usbdrd_dwc3_1 {
> > + snps,dis-u3-autosuspend-quirk;
> > + status = "okay";
> > +};
> > +
> > --
> > 2.17.1
> >
> >
> >
^ permalink raw reply
* Re: [PATCH v1 1/1] arm64: dts: rockchip: Add support for TB-96AI board
From: Manivannan Sadhasivam @ 2019-07-17 15:47 UTC (permalink / raw)
To: Elon Zhang
Cc: heiko, mark.rutland, robh+dt, linux-rockchip, linux-arm-kernel,
linux-kernel, devicetree
In-Reply-To: <20190711021209.32529-1-zhangzj@rock-chips.com>
Hi Elon,
Thanks for the patch. Overall, this patch needs a bit of cleanup. There
are many nodes added which are not available in mainline.
Please see comments inline.
On Thu, Jul 11, 2019 at 10:12:09AM +0800, Elon Zhang wrote:
> Add devicetree support for RK3399Pro TB-96AI board, one of
> the 96Boards family.
>
> The TB-96AI board is a 96Boards Compute SOM design, launched
> by Linaro, Rockchip and Beiqicloud.
>
> More information can be obtained from the following websites:
> 1.https://www.96boards.org/product/tb-96ai/
> 2.http://t.rock-chips.com/
> 3.http://www.beiqicloud.com/
>
> This patch add basic node for the board and support booting up
> to Fedora.
>
> Signed-off-by: Elon Zhang <zhangzj@rock-chips.com>
>
> diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
> index 5f2687acbf94..3d6c8d4363b5 100644
> --- a/arch/arm64/boot/dts/rockchip/Makefile
> +++ b/arch/arm64/boot/dts/rockchip/Makefile
> @@ -27,3 +27,4 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-rock960.dtb
> dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-rockpro64.dtb
> dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-sapphire.dtb
> dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-sapphire-excavator.dtb
> +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399pro-tb-96ai.dtb
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399pro-tb-96ai.dts b/arch/arm64/boot/dts/rockchip/rk3399pro-tb-96ai.dts
> new file mode 100644
> index 000000000000..1935df99065d
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3399pro-tb-96ai.dts
> @@ -0,0 +1,629 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (c) 2019 Fuzhou Rockchip Electronics Co., Ltd.
> + */
> +
> +/dts-v1/;
> +#include "rk3399pro.dtsi"
> +#include "rk3399-opp.dtsi"
> +
> +/ {
> + compatible = "rockchip,rk3399pro-tb-96ai", "rockchip,rk3399pro";
> +
I think the manufacturer of this board is, Xiamen Beiqi Technology Co. Ltd.
So, the compatible should be:
compatible = "beiqui,rk3399pro-tb-96ai", "rockchip,rk3399pro";
And there should a separate patch to add the vendor prefix. You can refer
below patch:
https://lkml.org/lkml/2019/7/17/39
> + chosen {
> + stdout-path = "serial2:1500000n8";
> + };
> +
> + xin32k: xin32k {
> + compatible = "fixed-clock";
> + clock-frequency = <32768>;
> + clock-output-names = "xin32k";
> + #clock-cells = <0>;
> + };
> +
> + vcc_phy: vcc-phy-regulator {
> + compatible = "regulator-fixed";
> + regulator-name = "vcc_phy";
> + regulator-always-on;
> + regulator-boot-on;
> + };
> +
> + vcc5v0_sys: vccsys {
> + compatible = "regulator-fixed";
> + regulator-name = "vcc5v0_sys";
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <5000000>;
> + regulator-max-microvolt = <5000000>;
> + };
> +
> + vdd_log: vdd_log {
> + compatible = "regulator-fixed";
> + regulator-name = "vdd_log";
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <900000>;
> + regulator-max-microvolt = <900000>;
> + };
> +
> + leds: gpio-leds {
> + compatible = "gpio-leds";
> + pinctrl-names = "default";
> + pinctrl-0 =<&leds_gpio>;
Leave a space after =
> +
> + led@1 {
> + gpios = <&gpio2 5 GPIO_ACTIVE_HIGH>;
> + label = "system_work_led1";
> + retain-state-suspended;
> + };
> +
We are using a standard LED formats for all 96Boards. Please see,
rk3399-rock960.dts for reference. Since there is only user leds
(apart from power leds), just define those as per the format.
> + led@2 {
> + gpios = <&gpio2 4 GPIO_ACTIVE_HIGH>;
> + label = "system_work_led2";
> + retain-state-suspended;
> + };
> +
> + led@3 {
> + gpios = <&gpio2 3 GPIO_ACTIVE_HIGH>;
> + label = "system_work_led3";
> + retain-state-suspended;
> + };
> + };
> +};
> +
> +&cpu_l0 {
> + cpu-supply = <&vdd_cpu_l>;
> +};
> +
> +&cpu_l1 {
> + cpu-supply = <&vdd_cpu_l>;
> +};
> +
> +&cpu_l2 {
> + cpu-supply = <&vdd_cpu_l>;
> +};
> +
> +&cpu_l3 {
> + cpu-supply = <&vdd_cpu_l>;
> +};
> +
> +&cpu_b0 {
> + cpu-supply = <&vdd_cpu_b>;
> +};
> +
> +&cpu_b1 {
> + cpu-supply = <&vdd_cpu_b>;
> +};
> +
> +&emmc_phy {
> + status = "okay";
> +};
> +
> +&i2c0 {
> + status = "okay";
> + i2c-scl-rising-time-ns = <180>;
> + i2c-scl-falling-time-ns = <30>;
> + clock-frequency = <400000>;
> +
> + rk809: pmic@20 {
> + compatible = "rockchip,rk809";
It looks like this driver is not present in mainline yet. I can see
some old patches in lkml archive but not sure about the status of those.
So, please remove this node and use dummy regulators where applicable.
> + reg = <0x20>;
> + interrupt-parent = <&gpio1>;
> + interrupts = <RK_PC2 IRQ_TYPE_LEVEL_LOW>;
> + pinctrl-names = "default", "pmic-sleep",
> + "pmic-power-off", "pmic-reset";
> + pinctrl-0 = <&pmic_int_l>;
> + pinctrl-1 = <&soc_slppin_slp>, <&rk809_slppin_slp>;
> + pinctrl-2 = <&soc_slppin_gpio>, <&rk809_slppin_pwrdn>;
> + pinctrl-3 = <&soc_slppin_gpio>,<&rk809_slppin_null>;
> + rockchip,system-power-controller;
> + pmic-reset-func = <1>;
> + wakeup-source;
> + #clock-cells = <1>;
> + clock-output-names = "rk808-clkout1", "rk808-clkout2";
> +
> + vcc1-supply = <&vcc5v0_sys>;
> + vcc2-supply = <&vcc5v0_sys>;
> + vcc3-supply = <&vcc5v0_sys>;
> + vcc4-supply = <&vcc5v0_sys>;
> + vcc5-supply = <&vcc_buck5>;
> + vcc6-supply = <&vcc_buck5>;
> + vcc7-supply = <&vcc3v3_sys>;
> + vcc8-supply = <&vcc3v3_sys>;
> + vcc9-supply = <&vcc5v0_sys>;
> +
> + pwrkey {
> + status = "okay";
> + };
> +
> + rtc {
> + status = "okay";
> + };
> +
> + pinctrl_rk8xx: pinctrl_rk8xx {
> + gpio-controller;
> + #gpio-cells = <2>;
> +
> + rk809_slppin_null: rk809_slppin_null {
> + pins = "gpio_slp";
> + function = "pin_fun0";
> + };
> +
> + rk809_slppin_slp: rk809_slppin_slp {
> + pins = "gpio_slp";
> + function = "pin_fun1";
> + };
> +
> + rk809_slppin_pwrdn: rk809_slppin_pwrdn {
> + pins = "gpio_slp";
> + function = "pin_fun2";
> + };
> +
> + rk809_slppin_rst: rk809_slppin_rst {
> + pins = "gpio_slp";
> + function = "pin_fun3";
> + };
> + };
> +
> + regulators {
> + vdd_center: DCDC_REG1 {
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <750000>;
> + regulator-max-microvolt = <1350000>;
> + regulator-initial-mode = <0x2>;
> + regulator-name = "vdd_center";
> + regulator-state-mem {
> + regulator-on-in-suspend;
> + regulator-suspend-microvolt = <900000>;
> + };
> + };
> +
> + vdd_cpu_l: DCDC_REG2 {
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <750000>;
> + regulator-max-microvolt = <1350000>;
> + regulator-ramp-delay = <6001>;
> + regulator-initial-mode = <0x2>;
> + regulator-name = "vdd_cpu_l";
> + regulator-state-mem {
> + regulator-off-in-suspend;
> + };
> + };
> +
> + vcc_ddr: DCDC_REG3 {
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-name = "vcc_ddr";
> + regulator-initial-mode = <0x2>;
> + regulator-state-mem {
> + regulator-on-in-suspend;
> + };
> + };
> +
> + vcc3v3_sys: DCDC_REG4 {
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-initial-mode = <0x2>;
> + regulator-name = "vcc3v3_sys";
> + regulator-state-mem {
> + regulator-on-in-suspend;
> + regulator-suspend-microvolt = <3300000>;
> + };
> + };
> +
> + vcc_buck5: DCDC_REG5 {
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <2200000>;
> + regulator-max-microvolt = <2200000>;
> + regulator-name = "vcc_buck5";
> + regulator-state-mem {
> + regulator-on-in-suspend;
> + regulator-suspend-microvolt = <2200000>;
> + };
> + };
> +
> + vcca_0v9: LDO_REG1 {
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <900000>;
> + regulator-max-microvolt = <900000>;
> + regulator-name = "vcca_0v9";
> + regulator-state-mem {
> + regulator-off-in-suspend;
> + };
> + };
> +
> + vcc_1v8: LDO_REG2 {
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> +
> + regulator-name = "vcc_1v8";
> + regulator-state-mem {
> + regulator-on-in-suspend;
> + regulator-suspend-microvolt = <1800000>;
> + };
> + };
> +
> + vcc0v9_soc: LDO_REG3 {
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <900000>;
> + regulator-max-microvolt = <900000>;
> +
> + regulator-name = "vcc0v9_soc";
> + regulator-state-mem {
> + regulator-on-in-suspend;
> + regulator-suspend-microvolt = <900000>;
> + };
> + };
> +
> + vcca_1v8: LDO_REG4 {
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> +
> + regulator-name = "vcca_1v8";
> + regulator-state-mem {
> + regulator-off-in-suspend;
> + };
> + };
> +
> + vdd1v5_dvp: LDO_REG5 {
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <1500000>;
> + regulator-max-microvolt = <1500000>;
> +
> + regulator-name = "vdd1v5_dvp";
> + regulator-state-mem {
> + regulator-off-in-suspend;
> + };
> + };
> +
> + vcc_1v5: LDO_REG6 {
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <1500000>;
> + regulator-max-microvolt = <1500000>;
> +
> + regulator-name = "vcc_1v5";
> + regulator-state-mem {
> + regulator-off-in-suspend;
> + };
> + };
> +
> + vcc_3v0: LDO_REG7 {
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <3000000>;
> + regulator-max-microvolt = <3000000>;
> +
> + regulator-name = "vcc_3v0";
> + regulator-state-mem {
> + regulator-off-in-suspend;
> + };
> + };
> +
> + vccio_sd: LDO_REG8 {
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <3300000>;
> +
> + regulator-name = "vccio_sd";
> + regulator-state-mem {
> + regulator-on-in-suspend;
> + regulator-suspend-microvolt = <3300000>;
> + };
> + };
> +
> + vcc_sd: LDO_REG9 {
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> +
> + regulator-name = "vcc_sd";
> + regulator-state-mem {
> + regulator-on-in-suspend;
> + regulator-suspend-microvolt = <3300000>;
> + };
> + };
> +
> + vcc5v0_usb: SWITCH_REG1 {
> + regulator-min-microvolt = <5000000>;
> + regulator-max-microvolt = <5000000>;
> +
> + regulator-name = "vcc5v0_usb";
> + regulator-state-mem {
> + regulator-off-in-suspend;
> + };
> + };
> +
> + vccio_3v3: SWITCH_REG2 {
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> +
> + regulator-name = "vccio_3v3";
> + regulator-state-mem {
> + regulator-off-in-suspend;
> + };
> + };
> + };
> + };
> +
> + vdd_cpu_b: tcs452x@1c {
> + compatible = "tcs,tcs452x";
Again, there is no driver for this.
> + reg = <0x1c>;
> + vin-supply = <&vcc5v0_sys>;
> + regulator-compatible = "fan53555-reg";
> + pinctrl-0 = <&vsel1_gpio>;
> + vsel-gpios = <&gpio1 RK_PC1 GPIO_ACTIVE_HIGH>;
> + regulator-name = "vdd_cpu_b";
> + regulator-min-microvolt = <712500>;
> + regulator-max-microvolt = <1500000>;
> + regulator-ramp-delay = <2300>;
> + fcs,suspend-voltage-selector = <1>;
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-initial-state = <3>;
> + regulator-state-mem {
> + regulator-off-in-suspend;
> + };
> + };
> +
> + vdd_gpu: tcs452x@10 {
> + compatible = "tcs,tcs452x";
ditto.
> + status = "okay";
> + reg = <0x10>;
> + vin-supply = <&vcc5v0_sys>;
> + regulator-compatible = "fan53555-reg";
> + pinctrl-0 = <&vsel2_gpio>;
> + vsel-gpios = <&gpio1 RK_PB6 GPIO_ACTIVE_HIGH>;
> + regulator-name = "vdd_gpu";
> + regulator-min-microvolt = <735000>;
> + regulator-max-microvolt = <1400000>;
> + regulator-ramp-delay = <2300>;
> + fcs,suspend-voltage-selector = <1>;
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-state-mem {
> + regulator-off-in-suspend;
> + };
> + };
> +
> +};
> +
> +&i2c8 {
> + status = "okay";
> + i2c-scl-rising-time-ns = <345>;
> + i2c-scl-falling-time-ns = <11>;
> + clock-frequency = <100000>;
> +
> + fusb0: fusb30x@22 {
> + compatible = "fairchild,fusb302";
ditto, please remove.
> + reg = <0x22>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&fusb0_int>;
> + int-n-gpios = <&gpio1 RK_PA2 GPIO_ACTIVE_HIGH>;
> + vbus-5v-gpios = <&gpio0 RK_PA1 GPIO_ACTIVE_LOW>;
> + status = "okay";
> + };
> +
> +};
> +
> +&io_domains {
> + status = "okay";
> + bt656-supply = <&vcca_1v8>; /* APIO2_VDD */
> + audio-supply = <&vcca_1v8>; /* APIO5_VDD */
> + sdmmc-supply = <&vccio_sd>; /* SDMMC0_VDD */
> + gpio1830-supply = <&vcc_1v8>; /* APIO4_VDD */
> +};
> +
> +&pinctrl {
> + pinctrl-names = "default";
> + pinctrl-0 = <&npu_ref_clk>;
> +
> + fusb30x {
> + fusb0_int: fusb0-int {
> + rockchip,pins =
> + <1 RK_PA2 0 &pcfg_pull_up>;
> + };
> + };
not needed.
> +
> + gpio-leds {
> + leds_gpio: leds-gpio {
> + rockchip,pins =
> + <2 5 RK_FUNC_GPIO &pcfg_pull_up>,
> + <2 4 RK_FUNC_GPIO &pcfg_pull_up>,
> + <2 3 RK_FUNC_GPIO &pcfg_pull_up>;
I don't think we need pull-up here.
> + };
> + };
> +
> + npu_clk {
> + npu_ref_clk: npu-ref-clk {
> + rockchip,pins =
> + <0 RK_PA2 1 &pcfg_pull_none>;
> + };
> + };
> +
> + pmic {
> + pmic_int_l: pmic-int-l {
> + rockchip,pins =
> + <1 RK_PC2 0 &pcfg_pull_up>;
> + };
> +
> + soc_slppin_gpio: soc-slppin-gpio {
> + rockchip,pins =
> + <1 RK_PA5 0 &pcfg_output_low>;
> + };
> +
> + soc_slppin_slp: soc-slppin-slp {
> + rockchip,pins =
> + <1 RK_PA5 1 &pcfg_pull_down>;
> + };
> +
> + vsel1_gpio: vsel1-gpio {
> + rockchip,pins =
> + <1 RK_PC1 0 &pcfg_pull_down>;
> + };
> +
> + vsel2_gpio: vsel2-gpio {
> + rockchip,pins =
> + <1 RK_PB6 0 &pcfg_pull_down>;
> + };
> + };
not needed.
> +
> + usb3 {
> + usb3_host_en: usb3-host-en {
> + rockchip,pins =
> + <2 RK_PA2 RK_FUNC_GPIO &pcfg_output_high>;
> + };
> + };
> +};
> +
> +&pmu_io_domains {
> + status = "okay";
> + pmu1830-supply = <&vcc_1v8>;
> +};
> +
> +&pwm0 {
> + status = "okay";
> +};
> +
> +&pwm2 {
> + status = "okay";
> +};
> +
> +&saradc {
> + status = "okay";
> + vref-supply = <&vcc_1v8>;
> +};
> +
> +&sdhci {
> + bus-width = <8>;
> + mmc-hs400-1_8v;
> + supports-emmc;
there is no such property, so please remove. Since this controller is
used for emmc, you can use "no-sd" and "no-sdio" properties if needed.
> + non-removable;
> + keep-power-in-suspend;
> + mmc-hs400-enhanced-strobe;
> + status = "okay";
> +};
> +
> +&sdmmc {
> + clock-frequency = <150000000>;
> + clock-freq-min-max = <400000 150000000>;
> + supports-sd;
Same as above, property not available. Use, "no-sdio" and "no-emmc" if
needed.
> + bus-width = <4>;
> + cap-mmc-highspeed;
> + cap-sd-highspeed;
> + disable-wp;
> + num-slots = <1>;
not needed.
> + vqmmc-supply = <&vccio_sd>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_cd &sdmmc_bus4>;
> + status = "okay";
> +};
> +
> +&tcphy0 {
> + extcon = <&fusb0>;
> + status = "okay";
> +};
> +
> +&tcphy1 {
> + status = "okay";
> +};
> +
> +&tsadc {
> + rockchip,hw-tshut-mode = <1>; /* tshut mode 0:CRU 1:GPIO */
> + rockchip,hw-tshut-polarity = <1>; /* tshut polarity 0:LOW 1:HIGH */
Any clue about shutdown temperature? Not necessary now but good to have.
Thanks,
Mani
> + status = "okay";
> +};
> +
> +&u2phy0 {
> + status = "okay";
> + extcon = <&fusb0>;
> +
> + u2phy0_otg: otg-port {
> + status = "okay";
> + };
> +
> + u2phy0_host: host-port {
> + phy-supply = <&vcc5v0_usb>;
> + status = "okay";
> + };
> +};
> +
> +&u2phy1 {
> + status = "okay";
> +
> + u2phy1_otg: otg-port {
> + status = "okay";
> + };
> +
> + u2phy1_host: host-port {
> + phy-supply = <&vcc5v0_usb>;
> + status = "okay";
> + };
> +};
> +
> +&uart0 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&uart0_xfer &uart0_cts>;
> + status = "okay";
> +};
> +
> +&uart2 {
> + status = "okay";
> +};
> +
> +&uart4 {
> + status = "okay";
> +};
> +
> +&usb_host0_ehci {
> + status = "okay";
> +};
> +
> +&usb_host1_ehci {
> + status = "okay";
> +};
> +
> +&usb_host0_ohci {
> + status = "okay";
> +};
> +
> +&usb_host1_ohci {
> + status = "okay";
> +};
> +
> +&usbdrd3_0 {
> + extcon = <&fusb0>;
> + status = "okay";
> +};
> +
> +&usbdrd3_1 {
> + status = "okay";
> + pinctrl-names = "default";
> + pinctrl-0 = <&usb3_host_en>;
> +};
> +
> +&usbdrd_dwc3_0 {
> + status = "okay";
> +};
> +
> +&usbdrd_dwc3_1 {
> + snps,dis-u3-autosuspend-quirk;
> + status = "okay";
> +};
> +
> --
> 2.17.1
>
>
>
^ permalink raw reply
* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Dmitry Osipenko @ 2019-07-17 15:17 UTC (permalink / raw)
To: Sowjanya Komatineni
Cc: Peter De Schrijver, Joseph Lo, thierry.reding, jonathanh, tglx,
jason, marc.zyngier, linus.walleij, stefan, mark.rutland,
pgaikwad, sboyd, linux-clk, linux-gpio, jckuo, talho, linux-tegra,
linux-kernel, mperttunen, spatra, robh+dt, devicetree
In-Reply-To: <6e73dcee-6e24-b646-97a4-4b34aedd231d@nvidia.com>
17.07.2019 9:36, Sowjanya Komatineni пишет:
>
> On 7/16/19 11:33 PM, Dmitry Osipenko wrote:
>> В Tue, 16 Jul 2019 22:55:52 -0700
>> Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
>>
>>> On 7/16/19 10:42 PM, Dmitry Osipenko wrote:
>>>> В Tue, 16 Jul 2019 22:25:25 -0700
>>>> Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
>>>>
>>>>> On 7/16/19 9:11 PM, Dmitry Osipenko wrote:
>>>>>> В Tue, 16 Jul 2019 19:35:49 -0700
>>>>>> Sowjanya Komatineni <skomatineni@nvidia.com> пишет:
>>>>>>
>>>>>>> On 7/16/19 7:18 PM, Sowjanya Komatineni wrote:
>>>>>>>> On 7/16/19 3:06 PM, Sowjanya Komatineni wrote:
>>>>>>>>> On 7/16/19 3:00 PM, Dmitry Osipenko wrote:
>>>>>>>>>> 17.07.2019 0:35, Sowjanya Komatineni пишет:
>>>>>>>>>>> On 7/16/19 2:21 PM, Dmitry Osipenko wrote:
>>>>>>>>>>>> 17.07.2019 0:12, Sowjanya Komatineni пишет:
>>>>>>>>>>>>> On 7/16/19 1:47 PM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>> 16.07.2019 22:26, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>> On 7/16/19 11:43 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>> 16.07.2019 21:30, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>>> On 7/16/19 11:25 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>>>> 16.07.2019 21:19, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>>>>> On 7/16/19 9:50 AM, Sowjanya Komatineni wrote:
>>>>>>>>>>>>>>>>>>>> On 7/16/19 8:00 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>>>>>>> 16.07.2019 11:06, Peter De Schrijver пишет:
>>>>>>>>>>>>>>>>>>>>>> On Tue, Jul 16, 2019 at 03:24:26PM +0800, Joseph
>>>>>>>>>>>>>>>>>>>>>> Lo wrote:
>>>>>>>>>>>>>>>>>>>>>>>> OK, Will add to CPUFreq driver...
>>>>>>>>>>>>>>>>>>>>>>>>> The other thing that also need attention is
>>>>>>>>>>>>>>>>>>>>>>>>> that T124 CPUFreq
>>>>>>>>>>>>>>>>>>>>>>>>> driver
>>>>>>>>>>>>>>>>>>>>>>>>> implicitly relies on DFLL driver to be probed
>>>>>>>>>>>>>>>>>>>>>>>>> first, which is
>>>>>>>>>>>>>>>>>>>>>>>>> icky.
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> Should I add check for successful dfll clk
>>>>>>>>>>>>>>>>>>>>>>>> register explicitly in
>>>>>>>>>>>>>>>>>>>>>>>> CPUFreq driver probe and defer till dfll clk
>>>>>>>>>>>>>>>>>>>>>>>> registers?
>>>>>>>>>>>>>>>>>>>>> Probably you should use the "device links". See
>>>>>>>>>>>>>>>>>>>>> [1][2] for the
>>>>>>>>>>>>>>>>>>>>> example.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> [1]
>>>>>>>>>>>>>>>>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/gpu/drm/tegra/dc.c#L2383
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> [2]
>>>>>>>>>>>>>>>>>>>>> https://www.kernel.org/doc/html/latest/driver-api/device_link.html
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Return EPROBE_DEFER instead of EINVAL if
>>>>>>>>>>>>>>>>>>>>> device_link_add() fails.
>>>>>>>>>>>>>>>>>>>>> And
>>>>>>>>>>>>>>>>>>>>> use of_find_device_by_node() to get the DFLL's
>>>>>>>>>>>>>>>>>>>>> device, see [3].
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> [3]
>>>>>>>>>>>>>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra20-devfreq.c#n100
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Will go thru and add...
>>>>>>>>>>>>>>>>>> Looks like I initially confused this case with getting
>>>>>>>>>>>>>>>>>> orphaned clock.
>>>>>>>>>>>>>>>>>> I'm now seeing that the DFLL driver registers the
>>>>>>>>>>>>>>>>>> clock and then
>>>>>>>>>>>>>>>>>> clk_get(dfll) should be returning EPROBE_DEFER until
>>>>>>>>>>>>>>>>>> DFLL driver is
>>>>>>>>>>>>>>>>>> probed, hence everything should be fine as-is and
>>>>>>>>>>>>>>>>>> there is no real
>>>>>>>>>>>>>>>>>> need
>>>>>>>>>>>>>>>>>> for the 'device link'. Sorry for the confusion!
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> Sorry, I didn't follow the mail thread. Just
>>>>>>>>>>>>>>>>>>>>>>> regarding the DFLL
>>>>>>>>>>>>>>>>>>>>>>> part.
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> As you know it, the DFLL clock is one of the CPU
>>>>>>>>>>>>>>>>>>>>>>> clock sources and
>>>>>>>>>>>>>>>>>>>>>>> integrated with DVFS control logic with the
>>>>>>>>>>>>>>>>>>>>>>> regulator. We will not
>>>>>>>>>>>>>>>>>>>>>>> switch
>>>>>>>>>>>>>>>>>>>>>>> CPU to other clock sources once we switched to
>>>>>>>>>>>>>>>>>>>>>>> DFLL. Because the
>>>>>>>>>>>>>>>>>>>>>>> CPU has
>>>>>>>>>>>>>>>>>>>>>>> been regulated by the DFLL HW with the DVFS table
>>>>>>>>>>>>>>>>>>>>>>> (CVB or OPP
>>>>>>>>>>>>>>>>>>>>>>> table
>>>>>>>>>>>>>>>>>>>>>>> you see
>>>>>>>>>>>>>>>>>>>>>>> in the driver.). We shouldn't reparent it to
>>>>>>>>>>>>>>>>>>>>>>> other sources with
>>>>>>>>>>>>>>>>>>>>>>> unknew
>>>>>>>>>>>>>>>>>>>>>>> freq/volt pair. That's not guaranteed to work. We
>>>>>>>>>>>>>>>>>>>>>>> allow switching to
>>>>>>>>>>>>>>>>>>>>>>> open-loop mode but different sources.
>>>>>>>>>>>>>>>>>>>>> Okay, then the CPUFreq driver will have to enforce
>>>>>>>>>>>>>>>>>>>>> DFLL freq to
>>>>>>>>>>>>>>>>>>>>> PLLP's
>>>>>>>>>>>>>>>>>>>>> rate before switching to PLLP in order to have a
>>>>>>>>>>>>>>>>>>>>> proper CPU voltage.
>>>>>>>>>>>>>>>>>>>> PLLP freq is safe to work for any CPU voltage. So no
>>>>>>>>>>>>>>>>>>>> need to enforce
>>>>>>>>>>>>>>>>>>>> DFLL freq to PLLP rate before changing CCLK_G source
>>>>>>>>>>>>>>>>>>>> to PLLP during
>>>>>>>>>>>>>>>>>>>> suspend
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Sorry, please ignore my above comment. During
>>>>>>>>>>>>>>>>>>> suspend, need to change
>>>>>>>>>>>>>>>>>>> CCLK_G source to PLLP when dfll is in closed loop
>>>>>>>>>>>>>>>>>>> mode first and
>>>>>>>>>>>>>>>>>>> then
>>>>>>>>>>>>>>>>>>> dfll need to be set to open loop.
>>>>>>>>>>>>>>>>>> Okay.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> And I don't exactly understand why we need to
>>>>>>>>>>>>>>>>>>>>>>> switch to PLLP in
>>>>>>>>>>>>>>>>>>>>>>> CPU
>>>>>>>>>>>>>>>>>>>>>>> idle
>>>>>>>>>>>>>>>>>>>>>>> driver. Just keep it on CL-DVFS mode all the
>>>>>>>>>>>>>>>>>>>>>>> time.
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> In SC7 entry, the dfll suspend function moves it
>>>>>>>>>>>>>>>>>>>>>>> the open-loop
>>>>>>>>>>>>>>>>>>>>>>> mode. That's
>>>>>>>>>>>>>>>>>>>>>>> all. The sc7-entryfirmware will handle the rest
>>>>>>>>>>>>>>>>>>>>>>> of the sequence to
>>>>>>>>>>>>>>>>>>>>>>> turn off
>>>>>>>>>>>>>>>>>>>>>>> the CPU power.
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> In SC7 resume, the warmboot code will handle the
>>>>>>>>>>>>>>>>>>>>>>> sequence to
>>>>>>>>>>>>>>>>>>>>>>> turn on
>>>>>>>>>>>>>>>>>>>>>>> regulator and power up the CPU cluster. And leave
>>>>>>>>>>>>>>>>>>>>>>> it on PLL_P.
>>>>>>>>>>>>>>>>>>>>>>> After
>>>>>>>>>>>>>>>>>>>>>>> resuming to the kernel, we re-init DFLL, restore
>>>>>>>>>>>>>>>>>>>>>>> the CPU clock
>>>>>>>>>>>>>>>>>>>>>>> policy (CPU
>>>>>>>>>>>>>>>>>>>>>>> runs on DFLL open-loop mode) and then moving to
>>>>>>>>>>>>>>>>>>>>>>> close-loop mode.
>>>>>>>>>>>>>>>>>>>>> The DFLL is re-inited after switching CCLK to DFLL
>>>>>>>>>>>>>>>>>>>>> parent during of
>>>>>>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>>>>>> early clocks-state restoring by CaR driver. Hence
>>>>>>>>>>>>>>>>>>>>> instead of having
>>>>>>>>>>>>>>>>>>>>> odd
>>>>>>>>>>>>>>>>>>>>> hacks in the CaR driver, it is much nicer to have a
>>>>>>>>>>>>>>>>>>>>> proper suspend-resume sequencing of the device
>>>>>>>>>>>>>>>>>>>>> drivers. In this case
>>>>>>>>>>>>>>>>>>>>> CPUFreq
>>>>>>>>>>>>>>>>>>>>> driver is the driver that enables DFLL and switches
>>>>>>>>>>>>>>>>>>>>> CPU to that
>>>>>>>>>>>>>>>>>>>>> clock
>>>>>>>>>>>>>>>>>>>>> source, which means that this driver is also should
>>>>>>>>>>>>>>>>>>>>> be responsible for
>>>>>>>>>>>>>>>>>>>>> management of the DFLL's state during of
>>>>>>>>>>>>>>>>>>>>> suspend/resume process. If
>>>>>>>>>>>>>>>>>>>>> CPUFreq driver disables DFLL during suspend and
>>>>>>>>>>>>>>>>>>>>> re-enables it
>>>>>>>>>>>>>>>>>>>>> during
>>>>>>>>>>>>>>>>>>>>> resume, then looks like the CaR driver hacks around
>>>>>>>>>>>>>>>>>>>>> DFLL are not
>>>>>>>>>>>>>>>>>>>>> needed.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> The DFLL part looks good to me. BTW, change the
>>>>>>>>>>>>>>>>>>>>>>> patch subject to
>>>>>>>>>>>>>>>>>>>>>>> "Add
>>>>>>>>>>>>>>>>>>>>>>> suspend-resume support" seems more appropriate to
>>>>>>>>>>>>>>>>>>>>>>> me.
>>>>>>>>>>>>>>>>>>>>>> To clarify this, the sequences for DFLL use are as
>>>>>>>>>>>>>>>>>>>>>> follows (assuming
>>>>>>>>>>>>>>>>>>>>>> all
>>>>>>>>>>>>>>>>>>>>>> required DFLL hw configuration has been done)
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Switch to DFLL:
>>>>>>>>>>>>>>>>>>>>>> 0) Save current parent and frequency
>>>>>>>>>>>>>>>>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>>>>>>>>>>>>>>>> 2) Enable DFLL
>>>>>>>>>>>>>>>>>>>>>> 3) Change cclk_g parent to DFLL
>>>>>>>>>>>>>>>>>>>>>> For OVR regulator:
>>>>>>>>>>>>>>>>>>>>>> 4) Change PWM output pin from tristate to output
>>>>>>>>>>>>>>>>>>>>>> 5) Enable DFLL PWM output
>>>>>>>>>>>>>>>>>>>>>> For I2C regulator:
>>>>>>>>>>>>>>>>>>>>>> 4) Enable DFLL I2C output
>>>>>>>>>>>>>>>>>>>>>> 6) Program DFLL to closed loop mode
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Switch away from DFLL:
>>>>>>>>>>>>>>>>>>>>>> 0) Change cclk_g parent to PLLP so the CPU
>>>>>>>>>>>>>>>>>>>>>> frequency is ok for
>>>>>>>>>>>>>>>>>>>>>> any
>>>>>>>>>>>>>>>>>>>>>> vdd_cpu voltage
>>>>>>>>>>>>>>>>>>>>>> 1) Program DFLL to open loop mode
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> I see during switch away from DFLL (suspend), cclk_g
>>>>>>>>>>>>>>>>>>> parent is not
>>>>>>>>>>>>>>>>>>> changed to PLLP before changing dfll to open loop
>>>>>>>>>>>>>>>>>>> mode.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Will add this ...
>>>>>>>>>>>>>>>>>> The CPUFreq driver switches parent to PLLP during the
>>>>>>>>>>>>>>>>>> probe, similar
>>>>>>>>>>>>>>>>>> should be done on suspend.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> I'm also wondering if it's always safe to switch to
>>>>>>>>>>>>>>>>>> PLLP in the probe.
>>>>>>>>>>>>>>>>>> If CPU is running on a lower freq than PLLP, then some
>>>>>>>>>>>>>>>>>> other more
>>>>>>>>>>>>>>>>>> appropriate intermediate parent should be selected.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> CPU parents are PLL_X, PLL_P, and dfll. PLL_X always
>>>>>>>>>>>>>>>>> runs at higher
>>>>>>>>>>>>>>>>> rate
>>>>>>>>>>>>>>>>> so switching to PLL_P during CPUFreq probe prior to
>>>>>>>>>>>>>>>>> dfll clock enable
>>>>>>>>>>>>>>>>> should be safe.
>>>>>>>>>>>>>>>> AFAIK, PLLX could run at ~200MHz. There is also a
>>>>>>>>>>>>>>>> divided output of
>>>>>>>>>>>>>>>> PLLP
>>>>>>>>>>>>>>>> which CCLKG supports, the PLLP_OUT4.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Probably, realistically, CPU is always running off a
>>>>>>>>>>>>>>>> fast PLLX during
>>>>>>>>>>>>>>>> boot, but I'm wondering what may happen on KEXEC. I
>>>>>>>>>>>>>>>> guess ideally CPUFreq driver should also have a
>>>>>>>>>>>>>>>> 'shutdown' callback to teardown DFLL
>>>>>>>>>>>>>>>> on a reboot, but likely that there are other
>>>>>>>>>>>>>>>> clock-related problems as
>>>>>>>>>>>>>>>> well that may break KEXEC and thus it is not very
>>>>>>>>>>>>>>>> important at the
>>>>>>>>>>>>>>>> moment.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> [snip]
>>>>>>>>>>>>>>> During bootup CPUG sources from PLL_X. By PLL_P source
>>>>>>>>>>>>>>> above I meant
>>>>>>>>>>>>>>> PLL_P_OUT4.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> As per clock policies, PLL_X is always used for high freq
>>>>>>>>>>>>>>> like
>>>>>>>>>>>>>>>> 800Mhz
>>>>>>>>>>>>>>> and for low frequency it will be sourced from PLLP.
>>>>>>>>>>>>>> Alright, then please don't forget to pre-initialize
>>>>>>>>>>>>>> PLLP_OUT4 rate to a
>>>>>>>>>>>>>> reasonable value using tegra_clk_init_table or
>>>>>>>>>>>>>> assigned-clocks.
>>>>>>>>>>>>> PLLP_OUT4 rate update is not needed as it is safe to run at
>>>>>>>>>>>>> 408Mhz because it is below fmax @ Vmin
>>>>>>>>>>>> So even 204MHz CVB entries are having the same voltage as
>>>>>>>>>>>> 408MHz, correct? It's not instantly obvious to me from the
>>>>>>>>>>>> DFLL driver's code where the fmax @ Vmin is defined, I see
>>>>>>>>>>>> that there is the min_millivolts
>>>>>>>>>>>> and frequency entries starting from 204MHZ defined
>>>>>>>>>>>> per-table.
>>>>>>>>>>> Yes at Vmin CPU Fmax is ~800Mhz. So anything below that will
>>>>>>>>>>> work at Vmin voltage and PLLP max is 408Mhz.
>>>>>>>>>> Thank you for the clarification. It would be good to have that
>>>>>>>>>> commented
>>>>>>>>>> in the code as well.
>>>>>>>>> OK, Will add...
>>>>>>>> Regarding, adding suspend/resume to CPUFreq, CPUFreq suspend
>>>>>>>> happens very early even before disabling non-boot CPUs and also
>>>>>>>> need to export clock driver APIs to CPUFreq.
>>>>>>>>
>>>>>>>> Was thinking of below way of implementing this...
>>>>>>>>
>>>>>>>>
>>>>>>>> Clock DFLL driver Suspend:
>>>>>>>>
>>>>>>>> - Save CPU clock policy registers, and Perform dfll
>>>>>>>> suspend which sets in open loop mode
>>>>>>>>
>>>>>>>> CPU Freq driver Suspend: does nothing
>>>>>>>>
>>>>>>>>
>>>>>>>> Clock DFLL driver Resume:
>>>>>>>>
>>>>>>>> - Re-init DFLL, Set in Open-Loop mode, restore CPU
>>>>>>>> Clock policy registers which actually sets source to DFLL along
>>>>>>>> with other CPU Policy register restore.
>>>>>>>>
>>>>>>>> CPU Freq driver Resume:
>>>>>>>>
>>>>>>>> - do clk_prepare_enable which acutally sets DFLL in
>>>>>>>> Closed loop mode
>>>>>>>>
>>>>>>>>
>>>>>>>> Adding one more note: Switching CPU Clock to PLLP is not needed
>>>>>>>> as CPU CLock can be from dfll in open-loop mode as DFLL is not
>>>>>>>> disabled anywhere throught the suspend/resume path and SC7 entry
>>>>>>>> FW and Warm boot code will switch CPU source to PLLP.
>>>>>> Since CPU resumes on PLLP, it will be cleaner to suspend it on
>>>>>> PLLP as well. And besides, seems that currently disabling DFLL
>>>>>> clock will disable DFLL completely and then you'd want to re-init
>>>>>> the DFLL on resume any ways. So better to just disable DFLL
>>>>>> completely on suspend, which should happen on clk_disable(dfll).
>>>>> Will switch to PLLP during CPUFreq suspend. With decision of using
>>>>> clk_disable during suspend, its mandatory to switch to PLLP as DFLL
>>>>> is completely disabled.
>>>>>
>>>>> My earlier concern was on restoring CPU policy as we can't do that
>>>>> from CPUFreq driver and need export from clock driver.
>>>>>
>>>>> Clear now and will do CPU clock policy restore in after dfll
>>>>> re-init.
>>>> Why the policy can't be saved/restored by the CaR driver as a
>>>> context of any other clock?
>>> restoring cpu clock policy involves programming source and
>>> super_cclkg_divider.
>>>
>>> cclk_g is registered as clk_super_mux and it doesn't use frac_div ops
>>> to do save/restore its divider.
>> That can be changed of course and I guess it also could be as simple as
>> saving and restoring of two raw u32 values of the policy/divider
>> registers.
>>
>>> Also, during clock context we cant restore cclk_g as cclk_g source
>>> will be dfll and dfll will not be resumed/re-initialized by the time
>>> clk_super_mux save/restore happens.
>>>
>>> we can't use save/restore context for dfll clk_ops because
>>> dfllCPU_out parent to CCLK_G is first in the clock tree and dfll_ref
>>> and dfll_soc peripheral clocks are not restored by the time dfll
>>> restore happens. Also dfll peripheral clock enables need to be
>>> restored before dfll restore happens which involves programming dfll
>>> controller for re-initialization.
>>>
>>> So dfll resume/re-init is done in clk-tegra210 at end of all clocks
>>> restore in V5 series but instead of in clk-tegra210 driver I moved
>>> now to dfll-fcpu driver pm_ops as all dfll dependencies will be
>>> restored thru clk_restore_context by then. This will be in V6.
>> Since DFLL is now guaranteed to be disabled across CaR suspend/resume
>> (hence it has nothing to do in regards to CCLK) and given that PLLs
>> state is restored before the rest of the clocks, I don't see why not to
>> implement CCLK save/restore in a generic fasion. CPU policy wull be
>> restored to either PLLP or PLLX (if CPUFreq driver is disabled).
>>
> CCLK_G save/restore should happen in clk_super_mux ops save/context and
> clk_super_mux save/restore happens very early as cclk_g is first in the
> clock tree and save/restore traverses through the tree top-bottom order.
If CCLK_G is restored before the PLLs, then just change the clocks order
such that it won't happen.
> DFLL enable thru CPUFreq resume happens after all clk_restore_context
> happens. So during clk_restore_context, dfll re-init doesnt happen and
> doing cpu clock policy restore during super_mux clk_ops will crash as
> DFLL is not initialized and its clock is not enabled but CPU clock
> restore sets source to DFLL if we restore during super_clk_mux
If CPU was suspended on PLLP, then it will be restored on PLLP by CaR. I
don't understand what DFLL has to do with the CCLK in that case during
the clocks restore.
^ permalink raw reply
* Re: [PATCH v5 03/11] driver core: Add sync_state driver/bus callback
From: Rob Herring @ 2019-07-17 15:01 UTC (permalink / raw)
To: Saravana Kannan
Cc: Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki, Frank Rowand,
devicetree, linux-kernel@vger.kernel.org, David Collins,
Android Kernel Team
In-Reply-To: <20190712235245.202558-4-saravanak@google.com>
On Fri, Jul 12, 2019 at 5:52 PM Saravana Kannan <saravanak@google.com> wrote:
>
> This sync_state driver/bus callback is called once all the consumers
> of a supplier have probed successfully.
>
> This allows the supplier device's driver/bus to sync the supplier
> device's state to the software state with the guarantee that all the
> consumers are actively managing the resources provided by the supplier
> device.
>
> To maintain backwards compatibility and ease transition from existing
> frameworks and resource cleanup schemes, late_initcall_sync is the
> earliest when the sync_state callback might be called.
>
> There is no upper bound on the time by which the sync_state callback
> has to be called. This is because if a consumer device never probes,
> the supplier has to maintain its resources in the state left by the
> bootloader. For example, if the bootloader leaves the display
> backlight at a fixed voltage and the backlight driver is never probed,
> you don't want the backlight to ever be turned off after boot up.
Doesn't putting the regulator in the simple-framebuffer node solve this problem in particular?
I think this suffers from the same problem we have today, but makes it per driver rather than global. We still have side effects of the h/w state being out of sync with the ref count. We really need to get rid of
^ permalink raw reply
* Re: [PATCH v5 02/11] of/platform: Add functional dependency link from DT bindings
From: Rob Herring @ 2019-07-17 14:35 UTC (permalink / raw)
To: Saravana Kannan
Cc: Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki, Frank Rowand,
Jonathan Corbet,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel@vger.kernel.org, David Collins, Android Kernel Team,
Linux Doc Mailing List
In-Reply-To: <CAGETcx-5ykD=9X1Lo2-G+T5uokFncbY2FmiJM8eZrgQ9JaBgxw@mail.gmail.com>
On Tue, Jul 16, 2019 at 5:54 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Tue, Jul 16, 2019 at 4:43 PM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Fri, Jul 12, 2019 at 5:52 PM Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > Add device-links after the devices are created (but before they are
> > > probed) by looking at common DT bindings like clocks and
> > > interconnects.
> > >
> > > Automatically adding device-links for functional dependencies at the
> > > framework level provides the following benefits:
> > >
> > > - Optimizes device probe order and avoids the useless work of
> > > attempting probes of devices that will not probe successfully
> > > (because their suppliers aren't present or haven't probed yet).
> > >
> > > For example, in a commonly available mobile SoC, registering just
> > > one consumer device's driver at an initcall level earlier than the
> > > supplier device's driver causes 11 failed probe attempts before the
> > > consumer device probes successfully. This was with a kernel with all
> > > the drivers statically compiled in. This problem gets a lot worse if
> > > all the drivers are loaded as modules without direct symbol
> > > dependencies.
> > >
> > > - Supplier devices like clock providers, interconnect providers, etc
> > > need to keep the resources they provide active and at a particular
> > > state(s) during boot up even if their current set of consumers don't
> > > request the resource to be active. This is because the rest of the
> > > consumers might not have probed yet and turning off the resource
> > > before all the consumers have probed could lead to a hang or
> > > undesired user experience.
> > >
> > > Some frameworks (Eg: regulator) handle this today by turning off
> > > "unused" resources at late_initcall_sync and hoping all the devices
> > > have probed by then. This is not a valid assumption for systems with
> > > loadable modules. Other frameworks (Eg: clock) just don't handle
> > > this due to the lack of a clear signal for when they can turn off
> > > resources. This leads to downstream hacks to handle cases like this
> > > that can easily be solved in the upstream kernel.
> > >
> > > By linking devices before they are probed, we give suppliers a clear
> > > count of the number of dependent consumers. Once all of the
> > > consumers are active, the suppliers can turn off the unused
> > > resources without making assumptions about the number of consumers.
> > >
> > > By default we just add device-links to track "driver presence" (probe
> > > succeeded) of the supplier device. If any other functionality provided
> > > by device-links are needed, it is left to the consumer/supplier
> > > devices to change the link when they probe.
> > >
> > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > ---
> > > .../admin-guide/kernel-parameters.txt | 5 ++
> > > drivers/of/platform.c | 57 +++++++++++++++++++
> > > 2 files changed, 62 insertions(+)
> > >
> > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > > index 138f6664b2e2..109b4310844f 100644
> > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > @@ -3141,6 +3141,11 @@
> > > This can be set from sysctl after boot.
> > > See Documentation/sysctl/vm.txt for details.
> > >
> > > + of_devlink [KNL] Make device links from common DT bindings. Useful
> > > + for optimizing probe order and making sure resources
> > > + aren't turned off before the consumer devices have
> > > + probed.
> > > +
> > > ohci1394_dma=early [HW] enable debugging via the ohci1394 driver.
> > > See Documentation/debugging-via-ohci1394.txt for more
> > > info.
> > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > > index 04ad312fd85b..0930f9f89571 100644
> > > --- a/drivers/of/platform.c
> > > +++ b/drivers/of/platform.c
> > > @@ -509,6 +509,62 @@ int of_platform_default_populate(struct device_node *root,
> > > }
> > > EXPORT_SYMBOL_GPL(of_platform_default_populate);
> > >
> > > +static int of_link_binding(struct device *dev,
> > > + const char *binding, const char *cell)
> > > +{
> > > + struct of_phandle_args sup_args;
> > > + struct platform_device *sup_dev;
> > > + unsigned int i = 0, links = 0;
> > > + u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
> > > +
> > > + while (!of_parse_phandle_with_args(dev->of_node, binding, cell, i,
> > > + &sup_args)) {
> > > + i++;
> > > + sup_dev = of_find_device_by_node(sup_args.np);
> > > + of_node_put(sup_args.np);
> > > + if (!sup_dev)
> > > + continue;
> > > + if (device_link_add(dev, &sup_dev->dev, dl_flags))
> > > + links++;
> > > + put_device(&sup_dev->dev);
> > > + }
> > > + if (links < i)
> > > + return -ENODEV;
> > > + return 0;
> > > +}
> > > +
> > > +static bool of_devlink;
> > > +core_param(of_devlink, of_devlink, bool, 0);
> > > +
> > > +/*
> > > + * List of bindings and their cell names (use NULL if no cell names) from which
> > > + * device links need to be created.
> > > + */
> > > +static const char * const link_bindings[] = {
> > > + "clocks", "#clock-cells",
> > > + "interconnects", "#interconnect-cells",
> > > +};
> > > +
> > > +static int of_link_to_suppliers(struct device *dev)
> > > +{
> > > + unsigned int i = 0;
> > > + bool done = true;
> > > +
> > > + if (!of_devlink)
> > > + return 0;
> > > + if (unlikely(!dev->of_node))
> > > + return 0;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(link_bindings) / 2; i++)
> > > + if (of_link_binding(dev, link_bindings[i * 2],
> > > + link_bindings[i * 2 + 1]))
> > > + done = false;
> >
> > Given the pending addition of regulators I think this should be
> > structured a bit differently so that we abstract out the matching and
> > phandle look-up so there's a clean separation of binding specifics.
> > It's kind of messy with 2 patterns to parse already and if we added a
> > 3rd? I would iterate over the properties as you do for regulators in
> > both cases and for each property call a binding specific match
> > function. The common pattern can of course be a common function. Let
> > me know if that makes sense. If not I can try to flesh it out some
> > more.
>
> I've added regulator support in this series and I've refactored this
> code as I went along. I fully expect to squash some of the refactors
> once the final result of the series is acceptable.
It would be easier to review the final result than incremental changes
which change the prior patches especially if the latter patches are
ultimately required.
> It's not clear to me if you got to the end of the series and still
> think the final result needs to be refactored. Let me know what you
> think about this towards the end of the series and I can clean it up
> if you still think it needs some clean up.
I probably should have replied on the regulator addition, but yes,
looking at that is what prompted my concerns here.
Rob
^ permalink raw reply
* Re: [PATCH v8 07/21] iommu/io-pgtable-arm-v7s: Extend MediaTek 4GB Mode
From: Will Deacon @ 2019-07-17 14:23 UTC (permalink / raw)
To: Yong Wu
Cc: youlin.pei, devicetree, Nicolas Boichat, cui.zhang,
srv_heupstream, chao.hao, Joerg Roedel, Will Deacon, linux-kernel,
Evan Green, Tomasz Figa, iommu, Rob Herring, linux-mediatek,
Matthias Brugger, yingjoe.chen, anan.sun, Robin Murphy,
Matthias Kaehlcke, linux-arm-kernel
In-Reply-To: <1563367459.31342.34.camel@mhfsdcap03>
On Wed, Jul 17, 2019 at 08:44:19PM +0800, Yong Wu wrote:
> On Mon, 2019-07-15 at 10:51 +0100, Will Deacon wrote:
> > On Sun, Jul 14, 2019 at 12:41:20PM +0800, Yong Wu wrote:
> > > @@ -742,7 +763,9 @@ static struct io_pgtable
> > > *arm_v7s_alloc_pgtable(struct io_pgtable_cfg *cfg,
> > > {
> > > struct arm_v7s_io_pgtable *data;
> > >
> > > - if (cfg->ias > ARM_V7S_ADDR_BITS || cfg->oas > ARM_V7S_ADDR_BITS)
> > > + if (cfg->ias > ARM_V7S_ADDR_BITS ||
> > > + (cfg->oas > ARM_V7S_ADDR_BITS &&
> > > + !(cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_EXT)))
> > > return NULL;
> >
> > I think you can rework this to do something like:
> >
> > if (cfg->ias > ARM_V7S_ADDR_BITS)
> > return NULL;
> >
> > if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_EXT) {
> > if (!IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT))
> > cfg->oas = min(cfg->oas, ARM_V7S_ADDR_BITS);
> > else if (cfg->oas > 34)
> > return NULL;
> > } else if (cfg->oas > ARM_V7S_ADDR_BITS) {
> > return NULL;
> > }
> >
> > so that we clamp the oas when phys_addr_t is 32-bit for you. That should
> > allow you to remove lots of the checking from iopte_to_paddr() too if you
> > check against oas in the map() function.
> >
> > Does that make sense?
>
> Of course I'm ok for this. I'm only afraid that this function has
> already 3 checking "if (x) return NULL", Here we add a new one and so
> many lines... Maybe the user should guarantee the right value of oas.
> How about move it into mtk_iommu.c?
>
> About the checking of iopte_to_paddr, I can not remove them. I know it
> may be a bit special and not readable. Hmm, I guess I should use a MACRO
> instead of the hard code 33 for the special 4GB mode case.
Why can't you just do something like:
if (!(cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_EXT))
return paddr;
if (pte & ARM_V7S_ATTR_MTK_PA_BIT33)
paddr |= BIT_ULL(33);
if (pte & ARM_V&S_ATTR_MTK_PA_BIT32)
paddr |= BIT_ULL(32);
return paddr;
The diff I sent previously sanitises the oas at init time, and then you
can just enforce it in map().
Will
^ permalink raw reply
* Re: [PATCH v2 1/4] opp: core: add regulators enable and disable
From: Kamil Konieczny @ 2019-07-17 14:14 UTC (permalink / raw)
To: Viresh Kumar
Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, Chanwoo Choi,
Krzysztof Kozlowski, Kukjin Kim, Kyungmin Park, Mark Rutland,
MyungJoo Ham, Nishanth Menon, Rob Herring, Stephen Boyd,
Viresh Kumar, devicetree, linux-arm-kernel, linux-kernel,
linux-pm, linux-samsung-soc
In-Reply-To: <20190716100539.4uqelbxqz7bmtmea@vireshk-i7>
On 16.07.2019 12:05, Viresh Kumar wrote:
> On 15-07-19, 14:04, Kamil Konieczny wrote:
>> Add enable regulators to dev_pm_opp_set_regulators() and disable
>> regulators to dev_pm_opp_put_regulators(). This prepares for
>> converting exynos-bus devfreq driver to use dev_pm_opp_set_rate().
>>
>> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
>> --
>> Changes in v2:
>>
>> - move regulator enable and disable into loop
>>
>> ---
>> drivers/opp/core.c | 18 +++++++++++++++---
>> 1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>> index 0e7703fe733f..069c5cf8827e 100644
>> --- a/drivers/opp/core.c
>> +++ b/drivers/opp/core.c
>> @@ -1570,6 +1570,10 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
>> goto free_regulators;
>> }
>>
>> + ret = regulator_enable(reg);
>> + if (ret < 0)
>> + goto disable;
>
> The name of this label is logically incorrect because we won't disable
> the regulator from there but put it. Over that, I would rather prefer
> to remove the label and add regulator_put() here itself.
I will change this and following according to your suggestions and will send v3.
>> +
>> opp_table->regulators[i] = reg;
>> }
>>
>> @@ -1582,9 +1586,15 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
>>
>> return opp_table;
>>
>> +disable:
>> + regulator_put(reg);
>> + --i;
>> +
>> free_regulators:
>> - while (i != 0)
>> - regulator_put(opp_table->regulators[--i]);
>> + for (; i >= 0; --i) {
>> + regulator_disable(opp_table->regulators[i]);
>> + regulator_put(opp_table->regulators[i]);
>
> This is incorrect as this will now try to put/disable the regulator
> which we failed to acquire. As --i happens only after the loop has run
> once. You can rather do:
>
> while (i--) {
> regulator_disable(opp_table->regulators[i]);
> regulator_put(opp_table->regulators[i]);
> }
>
>
>> + }
>>
>> kfree(opp_table->regulators);
>> opp_table->regulators = NULL;
>> @@ -1610,8 +1620,10 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table)
>> /* Make sure there are no concurrent readers while updating opp_table */
>> WARN_ON(!list_empty(&opp_table->opp_list));
>>
>> - for (i = opp_table->regulator_count - 1; i >= 0; i--)
>> + for (i = opp_table->regulator_count - 1; i >= 0; i--) {
>> + regulator_disable(opp_table->regulators[i]);
>> regulator_put(opp_table->regulators[i]);
>> + }
>>
>> _free_set_opp_data(opp_table);
>>
>> --
>> 2.22.0
>
--
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland
^ permalink raw reply
* Re: [PATCH v2 1/4] opp: core: add regulators enable and disable
From: Kamil Konieczny @ 2019-07-17 14:12 UTC (permalink / raw)
To: Chanwoo Choi
Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, Krzysztof Kozlowski,
Kukjin Kim, Kyungmin Park, Mark Rutland, MyungJoo Ham,
Nishanth Menon, Rob Herring, Stephen Boyd, Viresh Kumar,
devicetree, linux-arm-kernel, linux-kernel, linux-pm,
linux-samsung-soc
In-Reply-To: <9acc7dd0-614b-ccd3-a485-eeca3dab494b@samsung.com>
On 16.07.2019 06:03, Chanwoo Choi wrote:
> Hi Kamil,
>
> On 19. 7. 15. 오후 9:04, Kamil Konieczny wrote:
>> Add enable regulators to dev_pm_opp_set_regulators() and disable
>> regulators to dev_pm_opp_put_regulators(). This prepares for
>> converting exynos-bus devfreq driver to use dev_pm_opp_set_rate().
>
> IMHO, it is not proper to mention the specific driver name.
> If you explain the reason why enable the regulator before using it,
> it is enough description.
>
>>
>> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
>> --
>> Changes in v2:
>>
>> - move regulator enable and disable into loop
>>
>> ---
>> drivers/opp/core.c | 18 +++++++++++++++---
>> 1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>> index 0e7703fe733f..069c5cf8827e 100644
>> --- a/drivers/opp/core.c
>> +++ b/drivers/opp/core.c
>> @@ -1570,6 +1570,10 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
>> goto free_regulators;
>> }
>>
>> + ret = regulator_enable(reg);
>> + if (ret < 0)
>> + goto disable;
>> +
>> opp_table->regulators[i] = reg;
>> }
>>
>> @@ -1582,9 +1586,15 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
>>
>> return opp_table;
>>
>> +disable:
>> + regulator_put(reg);
>> + --i;
>> +
>> free_regulators:
>> - while (i != 0)
>> - regulator_put(opp_table->regulators[--i]);
>> + for (; i >= 0; --i) {
>> + regulator_disable(opp_table->regulators[i]);
>> + regulator_put(opp_table->regulators[i]);
>> + }
>>
>> kfree(opp_table->regulators);
>> opp_table->regulators = NULL;
>> @@ -1610,8 +1620,10 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table)
>> /* Make sure there are no concurrent readers while updating opp_table */
>> WARN_ON(!list_empty(&opp_table->opp_list));
>>
>> - for (i = opp_table->regulator_count - 1; i >= 0; i--)
>> + for (i = opp_table->regulator_count - 1; i >= 0; i--) {
>> + regulator_disable(opp_table->regulators[i]);
>> regulator_put(opp_table->regulators[i]);
>> + }
>>
>> _free_set_opp_data(opp_table);
>>
>>
>
> I agree to enable the regulator before using it.
> The bootloader might not enable the regulators
> and the kernel need to enable regulator in order to increase
> the reference count explicitly event if bootloader enables it.
>
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
Thank you, I will change commit description and send v3.
--
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland
^ permalink raw reply
* Re: [PATCH v3] arm64: dts: imx8mq: Add sai3 and sai6 nodes
From: Daniel Baluta @ 2019-07-17 14:04 UTC (permalink / raw)
To: Lucas Stach
Cc: Angus Ainslie, Mark Rutland, Devicetree List, Andra Danciu,
Abel Vesa, Anson Huang, Carlo Caione, andrew.smirnov,
Fabio Estevam, Sascha Hauer, Linux Kernel Mailing List,
Rob Herring, dl-linux-imx, Pengutronix Kernel Team, Shawn Guo,
Guido Günther, linux-arm-kernel
In-Reply-To: <1563292685.2676.12.camel@pengutronix.de>
On Tue, Jul 16, 2019 at 6:58 PM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Hi Daniel,
>
> Am Mittwoch, den 03.07.2019, 16:25 +0300 schrieb Daniel Baluta:
> > > On Wed, Jul 3, 2019 at 4:12 PM Angus Ainslie <angus@akkea.ca> wrote:
> > >
> > > Hi Daniel,
> > >
> > > On 2019-07-03 07:10, Daniel Baluta wrote:
> > > > > > > On Wed, Jul 3, 2019 at 4:01 PM Angus Ainslie <angus@akkea.ca> wrote:
> > > > >
> > > > > Hi Andra,
> > > > >
> > > > > I tried this out on linux-next and I'm not able to record or play
> > > > > sound.
> > > > >
> > > > > I also added the sai2 entry to test out our devkit and get a PCM
> > > > > timeout
> > > > > with that.
> > > >
> > > > Hi Angus,
> > > >
> > > > There are still lots of SAI patches that need to be upstream. Me and
> > > > Andra
> > > > will be working on that over this summer.
> > > >
> > > > >
> > > > > On 2019-07-02 07:23, Andra Danciu wrote:
> > > > > > SAI3 and SAI6 nodes are used to connect to an external codec.
> > > > > > They have 1 Tx and 1 Rx dataline.
> > > > > >
> > > > > > > > > > > Cc: Daniel Baluta <daniel.baluta@nxp.com>
> > > > > > > > > > > Signed-off-by: Andra Danciu <andradanciu1997@gmail.com>
> > > > > > ---
> > > > > > Changes since v2:
> > > > > > - removed multiple new lines
> > > > > >
> > > > > > Changes since v1:
> > > > > > - Added sai3 node because we need it to enable audio on pico-pi-8m
> > > > > > - Added commit description
> > > > > >
> > > > > > arch/arm64/boot/dts/freescale/imx8mq.dtsi | 29
> > > > > > +++++++++++++++++++++++++++++
> > > > > > 1 file changed, 29 insertions(+)
> > > > > >
> > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> > > > > > b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> > > > > > index d09b808eff87..736cf81b695e 100644
> > > > > > --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> > > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> > > > > > @@ -278,6 +278,20 @@
> > > > > > #size-cells = <1>;
> > > > > > ranges = <0x30000000 0x30000000 0x400000>;
> > > > > >
> > > > > > > > > > > + sai6: sai@30030000 {
> > > > > > + compatible = "fsl,imx8mq-sai",
> > > > >
> > > > > I don't find this compatible string in sound/soc/fsl/fsl_sai.c. Aren't
> > > > > the registers at a different offset from "fsl,imx6sx-sai".
> > > >
> > > > Yes, you are right on this. We are trying to slowly push all our
> > > > internal-tree
> > > > patches to mainline. Obviously, with started with low hanging fruits,
> > > > DTS
> > > > nodes and small SAI fixes.
> > > >
> > > > Soon, we will start to send patches for SAI IP ipgrade for imx8.
> > > >
> > > > >
> > > > > How is this supposed to work ?
> > > > >
> > > >
> > > > For the moment it won't work unless we will upstream all our SAI
> > > > internal patches.
> > > > But we will get there hopefully this summer.
> > > >
> > >
> > > Shouldn't a working driver be upstream before enabling it in the
> > > devicetree ?
> >
> > I see your point here and maybe your suggestion is the ideal
> > way to do things.
> >
> > Anyhow, I don't see a problem with adding the node in dts
> > because CONFIG_FSL_SAI is not set in the default config.
> >
> > We try to speedup the upstreaming process giving the fact
> > that SAI patches will go through audio maintainer's tree and
> > the DTS patches will most likely go through Shawn's tree.
>
> I've also looked at adding audio support to one of the custom boards I
> have here and was caught a bit off guard by the fact that the SAI
> driver is totally broken for i.MX8M due to missing patches, as I
> assumed the necessary bits are in place before the DT patches are
> landed. It's certainly not how things are usually done.
>
> This also means the DT description of the SAI nodes is wrong, as they
> are actually not compatible to the "fsl,imx6sx-sai". The register
> layout is moved around, so there is no point in claiming any backwards
> compat with the old SAI version.
>
> Do you have an ETA when the necessary patches for the i.MX8M SAI will
> be available for test and review?
No ETA for this. Sorry! We try to upstream it as soon as possible
^ permalink raw reply
* [PATCH v3 3/3] leds: Make led_set_brightness_sync() also use __led_set_brightness()
From: Jean-Jacques Hiblot @ 2019-07-17 13:59 UTC (permalink / raw)
To: jacek.anaszewski, pavel, robh+dt, mark.rutland, daniel.thompson
Cc: dmurphy, linux-leds, linux-kernel, devicetree,
Jean-Jacques Hiblot
In-Reply-To: <20190717135948.19340-1-jjhiblot@ti.com>
There are some LED drivers that do not implement brightness_set_blocking(),
for those drivers led_set_brightness_sync() cannot work.
Fixing it by calling first __led_set_brightness() and falling back to
__led_set_brightness_blocking() if it failed.
Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---
drivers/leds/led-core.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index dab32cf778f2..4a0506081c0e 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -320,15 +320,19 @@ int led_set_brightness_sync(struct led_classdev *led_cdev,
if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)
return -EBUSY;
- led_cdev->brightness = min(value, led_cdev->max_brightness);
+ value = min(value, led_cdev->max_brightness);
if (led_cdev->flags & LED_SUSPENDED)
return 0;
- ret = __led_set_brightness_blocking(led_cdev, led_cdev->brightness);
+ ret = __led_set_brightness(led_cdev, value);
+ if (ret == -ENOTSUPP)
+ ret = __led_set_brightness_blocking(led_cdev, value);
if (ret)
return ret;
+ led_cdev->brightness = value;
+
return __led_handle_regulator(led_cdev, led_cdev->brightness);
}
EXPORT_SYMBOL_GPL(led_set_brightness_sync);
--
2.17.1
^ permalink raw reply related
* [PATCH v3 2/3] leds: Add control of the voltage/current regulator to the LED core
From: Jean-Jacques Hiblot @ 2019-07-17 13:59 UTC (permalink / raw)
To: jacek.anaszewski, pavel, robh+dt, mark.rutland, daniel.thompson
Cc: dmurphy, linux-leds, linux-kernel, devicetree,
Jean-Jacques Hiblot
In-Reply-To: <20190717135948.19340-1-jjhiblot@ti.com>
Sometimes LEDs are powered by an external voltage/current regulator. Let
the LED core know about it. This allows the LED core to turn on or off
managed power supplies.
Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
Reviewed-by: Dan Murphy <dmurphy@ti.com>
---
drivers/leds/led-class.c | 15 +++++++++++++
drivers/leds/led-core.c | 47 +++++++++++++++++++++++++++++++++++++---
drivers/leds/leds.h | 1 +
include/linux/leds.h | 4 ++++
4 files changed, 64 insertions(+), 3 deletions(-)
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 4793e77808e2..cadd43c30d50 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -253,6 +253,7 @@ int of_led_classdev_register(struct device *parent, struct device_node *np,
{
char name[LED_MAX_NAME_SIZE];
int ret;
+ struct regulator *regulator;
ret = led_classdev_next_name(led_cdev->name, name, sizeof(name));
if (ret < 0)
@@ -272,6 +273,20 @@ int of_led_classdev_register(struct device *parent, struct device_node *np,
dev_warn(parent, "Led %s renamed to %s due to name collision",
led_cdev->name, dev_name(led_cdev->dev));
+ regulator = devm_regulator_get_optional(led_cdev->dev, "power");
+ if (IS_ERR(regulator)) {
+ if (PTR_ERR(regulator) != -ENODEV) {
+ dev_err(led_cdev->dev, "Cannot get the power supply for %s\n",
+ led_cdev->name);
+ device_unregister(led_cdev->dev);
+ mutex_unlock(&led_cdev->led_access);
+ return PTR_ERR(regulator);
+ }
+ led_cdev->regulator = NULL;
+ } else {
+ led_cdev->regulator = regulator;
+ }
+
if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) {
ret = led_add_brightness_hw_changed(led_cdev);
if (ret) {
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 7107cd7e87cf..dab32cf778f2 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -23,6 +23,33 @@ EXPORT_SYMBOL_GPL(leds_list_lock);
LIST_HEAD(leds_list);
EXPORT_SYMBOL_GPL(leds_list);
+static bool __led_need_regulator_update(struct led_classdev *led_cdev,
+ int brightness)
+{
+ bool new_state = (brightness != LED_OFF);
+
+ return led_cdev->regulator && led_cdev->regulator_state != new_state;
+}
+
+static int __led_handle_regulator(struct led_classdev *led_cdev,
+ int brightness)
+{
+ int rc;
+
+ if (__led_need_regulator_update(led_cdev, brightness)) {
+
+ if (brightness != LED_OFF)
+ rc = regulator_enable(led_cdev->regulator);
+ else
+ rc = regulator_disable(led_cdev->regulator);
+ if (rc)
+ return rc;
+
+ led_cdev->regulator_state = (brightness != LED_OFF);
+ }
+ return 0;
+}
+
static int __led_set_brightness(struct led_classdev *led_cdev,
enum led_brightness value)
{
@@ -115,6 +142,8 @@ static void set_brightness_delayed(struct work_struct *ws)
if (ret == -ENOTSUPP)
ret = __led_set_brightness_blocking(led_cdev,
led_cdev->delayed_set_value);
+ __led_handle_regulator(led_cdev, led_cdev->delayed_set_value);
+
if (ret < 0 &&
/* LED HW might have been unplugged, therefore don't warn */
!(ret == -ENODEV && (led_cdev->flags & LED_UNREGISTERING) &&
@@ -256,8 +285,14 @@ void led_set_brightness_nopm(struct led_classdev *led_cdev,
enum led_brightness value)
{
/* Use brightness_set op if available, it is guaranteed not to sleep */
- if (!__led_set_brightness(led_cdev, value))
- return;
+ if (!__led_set_brightness(led_cdev, value)) {
+ /*
+ * if regulator state doesn't need to be changed, that is all/
+ * Otherwise delegate the change to a work queue
+ */
+ if (!__led_need_regulator_update(led_cdev, value))
+ return;
+ }
/* If brightness setting can sleep, delegate it to a work queue task */
led_cdev->delayed_set_value = value;
@@ -280,6 +315,8 @@ EXPORT_SYMBOL_GPL(led_set_brightness_nosleep);
int led_set_brightness_sync(struct led_classdev *led_cdev,
enum led_brightness value)
{
+ int ret;
+
if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)
return -EBUSY;
@@ -288,7 +325,11 @@ int led_set_brightness_sync(struct led_classdev *led_cdev,
if (led_cdev->flags & LED_SUSPENDED)
return 0;
- return __led_set_brightness_blocking(led_cdev, led_cdev->brightness);
+ ret = __led_set_brightness_blocking(led_cdev, led_cdev->brightness);
+ if (ret)
+ return ret;
+
+ return __led_handle_regulator(led_cdev, led_cdev->brightness);
}
EXPORT_SYMBOL_GPL(led_set_brightness_sync);
diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
index 47b229469069..5aa5c038bd38 100644
--- a/drivers/leds/leds.h
+++ b/drivers/leds/leds.h
@@ -11,6 +11,7 @@
#include <linux/rwsem.h>
#include <linux/leds.h>
+#include <linux/regulator/consumer.h>
static inline int led_get_brightness(struct led_classdev *led_cdev)
{
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 9b2bf574a17a..bee8e3f8dddd 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -123,6 +123,10 @@ struct led_classdev {
/* Ensures consistent access to the LED Flash Class device */
struct mutex led_access;
+
+ /* regulator */
+ struct regulator *regulator;
+ bool regulator_state;
};
extern int of_led_classdev_register(struct device *parent,
--
2.17.1
^ permalink raw reply related
* [PATCH v3 1/3] dt-bindings: leds: document the "power-supply" property
From: Jean-Jacques Hiblot @ 2019-07-17 13:59 UTC (permalink / raw)
To: jacek.anaszewski, pavel, robh+dt, mark.rutland, daniel.thompson
Cc: dmurphy, linux-leds, linux-kernel, devicetree,
Jean-Jacques Hiblot
In-Reply-To: <20190717135948.19340-1-jjhiblot@ti.com>
Sometimes LEDs are powered by a voltage/current regulator. Describing it
in the device-tree makes it possible for the LED core to enable/disable it
when needed.
Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
Reviewed-by: Dan Murphy <dmurphy@ti.com>
---
Documentation/devicetree/bindings/leds/common.txt | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
index 70876ac11367..f496ec1c1542 100644
--- a/Documentation/devicetree/bindings/leds/common.txt
+++ b/Documentation/devicetree/bindings/leds/common.txt
@@ -61,6 +61,9 @@ Optional properties for child nodes:
- panic-indicator : This property specifies that the LED should be used,
if at all possible, as a panic indicator.
+- power-supply : Is a phandle to a voltage/current regulator used to to power
+ the LED.
+
- trigger-sources : List of devices which should be used as a source triggering
this LED activity. Some LEDs can be related to a specific
device and should somehow indicate its state. E.g. USB 2.0
@@ -106,6 +109,7 @@ gpio-leds {
label = "Status";
linux,default-trigger = "heartbeat";
gpios = <&gpio0 0 GPIO_ACTIVE_HIGH>;
+ power-supply = <&led_regulator>;
};
usb {
--
2.17.1
^ permalink raw reply related
* [PATCH v3 0/3] leds: Add control of the voltage/current regulator to the LED core
From: Jean-Jacques Hiblot @ 2019-07-17 13:59 UTC (permalink / raw)
To: jacek.anaszewski, pavel, robh+dt, mark.rutland, daniel.thompson
Cc: dmurphy, linux-leds, linux-kernel, devicetree,
Jean-Jacques Hiblot
This series makes it possible for the LED core to manage the power supply
of a LED. It uses the regulator API to disable/enable the power if when the
LED is turned on/off.
This is especially useful in situations where the LED driver/controller is
not supplying the power.
While at it, throw in a fix for led_set_brightness_sync() so that it can
work with drivers that don't provide brightness_set_blocking()
changes in v3:
- reword device-tree description
- reword commit log
- remove regulator updates from functions used in atomic context. If the
regulator must be updated, it is defered to a workqueue.
- Fix led_set_brightness_sync() to work with the non-blocking function
__led_set_brightness()
changes in v2:
- use devm_regulator_get_optional() to avoid using the dummy regulator and
do some unnecessary work
Jean-Jacques Hiblot (3):
dt-bindings: leds: document the "power-supply" property
leds: Add control of the voltage/current regulator to the LED core
leds: Make led_set_brightness_sync() also use __led_set_brightness()
.../devicetree/bindings/leds/common.txt | 4 ++
drivers/leds/led-class.c | 15 ++++++
drivers/leds/led-core.c | 53 +++++++++++++++++--
drivers/leds/leds.h | 1 +
include/linux/leds.h | 4 ++
5 files changed, 73 insertions(+), 4 deletions(-)
--
2.17.1
^ permalink raw reply
* Re: [PATCH V4 2/2] gpio: inverter: document the inverter bindings
From: Harish Jenny K N @ 2019-07-17 13:51 UTC (permalink / raw)
To: Rob Herring, Linus Walleij
Cc: Bartosz Golaszewski, Mark Rutland, devicetree,
open list:GPIO SUBSYSTEM <linux-gpio@vger.kernel.org>, Balasubramani Vivekanandan
In-Reply-To: <f1616784-4dbf-d0fa-b33e-c85fd569383a@mentor.com>
Hi Linus,
On 10/07/19 1:58 PM, Harish Jenny K N wrote:
> Hi,
>
> On 09/07/19 9:38 PM, Rob Herring wrote:
>> On Mon, Jul 8, 2019 at 11:25 PM Harish Jenny K N
>> <harish_kandiga@mentor.com> wrote:
>>> Hi Rob,
>>>
>>>
>>> On 09/07/19 4:06 AM, Rob Herring wrote:
>>>> On Fri, Jun 28, 2019 at 3:31 AM Harish Jenny K N
>>>> <harish_kandiga@mentor.com> wrote:
>>>>> Document the device tree binding for the inverter gpio
>>>>> controller to configure the polarity of the gpio pins
>>>>> used by the consumers.
>>>>>
>>>>> Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>
>>>>> ---
>>>>> .../devicetree/bindings/gpio/gpio-inverter.txt | 29 ++++++++++++++++++++++
>>>>> 1 file changed, 29 insertions(+)
>>>>> create mode 100644 Documentation/devicetree/bindings/gpio/gpio-inverter.txt
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-inverter.txt b/Documentation/devicetree/bindings/gpio/gpio-inverter.txt
>>>>> new file mode 100644
>>>>> index 0000000..8bb6b2e
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/gpio/gpio-inverter.txt
>>>>> @@ -0,0 +1,29 @@
>>>>> +GPIO-INVERTER
>>>>> +======
>>>>> +This binding defines the gpio-inverter. The gpio-inverter is a driver that
>>>>> +allows to properly describe the gpio polarities on the hardware.
>>>> I don't understand. Please explain this in terms of the hardware, not a driver.
>>> gpio inverters can be used on different hardware to alter the polarity of gpio chips.
>>> The polarity of pins can change from hardware to hardware with the use of inverters.
>> Yes, I know what an inverter is.
>>
>>> This device tree binding models gpio inverters in the device tree to properly describe the hardware.
>> We already define the active state of GPIOs in the consumers. If
>> there's an inverter in the middle, the consumer active state is simply
>> inverted. I don't agree that that is a hack as Linus said without some
>> reasoning why an inverter needs to be modeled in DT. Anything about
>> what 'userspace' needs is not a reason. That's a Linux thing that has
>> little to do with hardware description.
>
> Yes we are talking about the hardware level inversions here. The usecase is for those without the gpio consumer driver. The usecase started with the concept of allowing an abstraction of the underlying hardware for the userland controlling program such that this program does not care whether the GPIO lines are inverted or not physically. In other words, a single userland controlling program can work unmodified across a variety of hardware platforms with the device tree mapping the logical to physical relationship of the GPIO hardware.
> I totally understand anything about what 'userspace' needs is not a reason, but this is not restricted to userspace alone as kernel drivers may need this just as much. Also we are just modelling/describing the hardware state in the device tree.
>
> Just to mention that Linus Walleij had proposed this inverter model to describe the hardware and the gpio inverter driver is developed based on comments/review from him.
>
> Also my sincere request to Linus Walleij to please let his opinion know on this.
>
> Thanks,
>
> Best Regards,
> Harish Jenny K N
Can you please give your opinion on this.
Thanks.
Best Regards,
Harish Jenny K N
>
^ permalink raw reply
* Re: [PATCH v2 2/2] leds: Add control of the voltage/current regulator to the LED core
From: Jean-Jacques Hiblot @ 2019-07-17 13:47 UTC (permalink / raw)
To: Daniel Thompson
Cc: jacek.anaszewski, pavel, robh+dt, mark.rutland, dmurphy,
linux-leds, linux-kernel, devicetree
In-Reply-To: <20190716105032.thpcttko5do3u56n@holly.lan>
On 16/07/2019 12:50, Daniel Thompson wrote:
> On Mon, Jul 15, 2019 at 05:56:57PM +0200, Jean-Jacques Hiblot wrote:
>> A LED is usually powered by a voltage/current regulator. Let the LED core
> This is almost certainly nitpicking but since there's enough other
> review comments that you will have to respin anyway ;-)
No problems. comments are welcome.
> Is an LED really "usually powered by a voltage/current regulator"? Some
> LEDs have a software controlled power supply but I'm not sure it is
> the usual case.
True. I'll reword this.
> Likewise its a little confusing to be talking about LEDs with an
> external current regulator since, although that is possible, it is also
> one the main features provided by LED driver chips.
In my experience LED drivers are quite often current sinks. In that case
the power is provided externally, and sometimes by a managed regulator.
Thanks,
JJ
>
> Daniel.
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: mmc: Document Aspeed SD controller
From: Rob Herring @ 2019-07-17 13:43 UTC (permalink / raw)
To: Andrew Jeffery
Cc: linux-mmc, Ulf Hansson, Mark Rutland, Joel Stanley, Adrian Hunter,
devicetree,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-aspeed, linux-kernel@vger.kernel.org, Ryan Chen
In-Reply-To: <e7b472a8-73ae-4f39-a3e4-9e2d9dbcd01e@www.fastmail.com>
On Tue, Jul 16, 2019 at 9:58 PM Andrew Jeffery <andrew@aj.id.au> wrote:
>
>
>
> On Wed, 17 Jul 2019, at 00:27, Rob Herring wrote:
> > On Mon, Jul 15, 2019 at 6:36 PM Andrew Jeffery <andrew@aj.id.au> wrote:
> > >
> > >
> > >
> > > On Tue, 16 Jul 2019, at 07:47, Rob Herring wrote:
> > > > On Thu, Jul 11, 2019 at 9:32 PM Andrew Jeffery <andrew@aj.id.au> wrote:
> > > > >
> > > > > The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the
> > > > > SDIO Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit
> > > > > data bus if only a single slot is enabled.
> > > > >
> > > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > > > > ---
> > > > > In v2:
> > > > >
> > > > > * Rename to aspeed,sdhci.yaml
> > > > > * Rename sd-controller compatible
> > > > > * Add `maxItems: 1` for reg properties
> > > > > * Move sdhci subnode description to patternProperties
> > > > > * Drop sdhci compatible requirement
> > > > > * #address-cells and #size-cells are required
> > > > > * Prevent additional properties
> > > > > * Implement explicit ranges in example
> > > > > * Remove slot property
> > > > >
> > > > > .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 90 +++++++++++++++++++
> > > > > 1 file changed, 90 insertions(+)
> > > > > create mode 100644 Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > > > new file mode 100644
> > > > > index 000000000000..67a691c3348c
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > > > @@ -0,0 +1,90 @@
> > > > > +# SPDX-License-Identifier: GPL-2.0-or-later
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id: http://devicetree.org/schemas/mmc/aspeed,sdhci.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: ASPEED SD/SDIO/eMMC Controller
> > > > > +
> > > > > +maintainers:
> > > > > + - Andrew Jeffery <andrew@aj.id.au>
> > > > > + - Ryan Chen <ryanchen.aspeed@gmail.com>
> > > > > +
> > > > > +description: |+
> > > > > + The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the SDIO
> > > > > + Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit data bus if
> > > > > + only a single slot is enabled.
> > > > > +
> > > > > + The two slots are supported by a common configuration area. As the SDHCIs for
> > > > > + the slots are dependent on the common configuration area, they are described
> > > > > + as child nodes.
> > > > > +
> > > > > +properties:
> > > > > + compatible:
> > > > > + enum: [ aspeed,ast2400-sd-controller, aspeed,ast2500-sd-controller ]
> > > >
> > > > This is actually a list of 4 strings. Please reformat to 1 per line.
> > >
> > > On reflection that's obvious, but also a somewhat subtle interaction with the
> > > preference for no quotes (the obvious caveat being "except where required").
> >
> > It wasn't something I'd run into before. I'm working on a check, but
> > unfortunately we can only check for quotes not needed and can't check
> > for missing quotes.
> >
> > > Thanks for pointing it out.
> > >
> > > I have been running `make dt_binding_check` and `make dtbs_check` over
> > > these, looks like I need to up my game a bit though. Do you do additional things
> > > in your workflow?
> >
> > That should have thrown the warnings. If you aren't seeing those, do
> > you have dtschema package installed (see
> > Documentation/devicetree/writing-schema.md)?
>
> I do have it installed, but as mentioned previously there's a fair few
> warnings emitted currently by the Aspeed devicetrees, so it might have
> got lost in the noise. I've started to clean that up, though probably need
> some direction there too.
>
> Separately I'm currently trying to track down an issue where I get errors
> on the Aspeed dts cpu nodes about failing to match the riscv CPU
> compatibles, it seems dt-validate isn't finding the ARM CPU compatible
> strings. It feels more annoying to track down that I'd like.
There's a fix in today's linux-next for that and it should be in
Linus' tree in a few days.
Rob
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: i2c: Add MediaTek i2c AC timing binding
From: Wolfram Sang @ 2019-07-17 13:29 UTC (permalink / raw)
To: Qii Wang
Cc: linux-i2c, devicetree, linux-arm-kernel, linux-kernel,
linux-mediatek, srv_heupstream, leilk.liu, matthias.bgg, robh+dt
In-Reply-To: <1563368121.16970.7.camel@mhfsdcap03>
[-- Attachment #1: Type: text/plain, Size: 314 bytes --]
> > Can't you use those to derive your values from that? Which ones are you missing
> > if not?
>
> I have take a little time to develop a new patch which based on your
> suggestions, and it tested OK. Thanks for your suggestions, I will
> update the patch after I test it fully.
Great news, thanks!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 2/2] leds: Add control of the voltage/current regulator to the LED core
From: Jean-Jacques Hiblot @ 2019-07-17 13:14 UTC (permalink / raw)
To: Dan Murphy, jacek.anaszewski, pavel, robh+dt, mark.rutland,
daniel.thompson
Cc: linux-leds, linux-kernel, devicetree
In-Reply-To: <9c17e9cc-8162-7b5d-2db9-d2bed20969c5@ti.com>
Hi Dan,
On 15/07/2019 20:59, Dan Murphy wrote:
> JJ
>
> On 7/15/19 10:56 AM, Jean-Jacques Hiblot wrote:
>> A LED is usually powered by a voltage/current regulator. Let the LED
>> core
>> know about it. This allows the LED core to turn on or off the power
>> supply
>> as needed.
>
> This allows the LED core to turn on or off managed power supplies.
>
>
>>
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>> ---
>> if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) {
>> ret = led_add_brightness_hw_changed(led_cdev);
>> if (ret) {
>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
>> index 7107cd7e87cf..a12b880b0a2f 100644
>> --- a/drivers/leds/led-core.c
>> +++ b/drivers/leds/led-core.c
>> @@ -23,6 +23,33 @@ EXPORT_SYMBOL_GPL(leds_list_lock);
>> LIST_HEAD(leds_list);
>> EXPORT_SYMBOL_GPL(leds_list);
>> +static bool __led_need_regulator_update(struct led_classdev
>> *led_cdev,
>> + int brightness)
>> +{
>> + bool new_state = (brightness != LED_OFF);
>> +
>> + return led_cdev->regulator && led_cdev->regulator_state !=
>> new_state;
>> +}
>> +
>> +static int __led_handle_regulator(struct led_classdev *led_cdev,
>> + int brightness)
>> +{
>> + int rc;
>
> Should there be a check for the regulator pointer.
>
> If (!led_cdev->regulator)
>
> return 0;
Not required because __led_need_regulator_update() returns false if
led_cdev->regulator is NULL.
Thanks for the review
JJ
>
>
> Otherwise
>
> Reviewed-by: Dan Murphy <dmurphy@ti.com>
>
> <snip>
>
^ permalink raw reply
* Re: [PATCH v1 37/50] ARM: dts: exynos: change parent and rate of bus_fsys in Exynos5422
From: Lukasz Luba @ 2019-07-17 12:55 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: devicetree, linux-kernel, linux-arm-kernel,
linux-samsung-soc@vger.kernel.org, linux-clk, mturquette, sboyd,
Bartłomiej Żołnierkiewicz, kgene, mark.rutland,
robh+dt, Chanwoo Choi, kyungmin.park, Andrzej Hajda,
Marek Szyprowski, s.nawrocki, myungjoo.ham
In-Reply-To: <CAJKOXPfDX06s7eMctbnPabxho2EaWcTM4xAGKCd_+O6jCCDcRQ@mail.gmail.com>
On 7/17/19 1:11 PM, Krzysztof Kozlowski wrote:
> On Wed, 17 Jul 2019 at 13:06, Lukasz Luba <l.luba@partner.samsung.com> wrote:
>>
>>
>>
>> On 7/17/19 12:45 PM, Krzysztof Kozlowski wrote:
>>> On Wed, 17 Jul 2019 at 12:39, Lukasz Luba <l.luba@partner.samsung.com> wrote:
>>>>>>
>>>>>> &bus_fsys {
>>>>>> devfreq = <&bus_wcore>;
>>>>>> + assigned-clocks = <&clock CLK_MOUT_ACLK200_FSYS>,
>>>>>> + <&clock CLK_DOUT_ACLK200_FSYS>,
>>>>>> + <&clock CLK_FOUT_DPLL>;
>>>>>> + assigned-clock-parents = <&clock CLK_MOUT_SCLK_DPLL>;
>>>>>> + assigned-clock-rates = <0>, <240000000>,<1200000000>;
>>>>>
>>>>> Here and in all other patches:
>>>>> I am not entirely sure that this should be here. It looks like
>>>>> property of the SoC. Do we expect that buses will be configured to
>>>>> different clock rates between different boards?
This is the board file for Exynos5420/5422/5800 which enables buses.
Thus, I have change them here. Patch 49/50 adds these buses to
Exynos5800 (Peach Pi). In Exynos5420 there is no clock tree for
bus_isp266. The parents for different devices could be also different.
It is because i.e. in 5420 there is 2 bit in the WCORE 1st mux while in
5422 there is 3 bits (6 parents possible).
That's why I have picked exynos5422-odroid-core.dtsi to reference
the bus devices and pinned them into proper parent and changed rate.
When you check patch 49/50 for 5800 not all the parents are the same.
(1) I could create a dedicated files like: exynos5422-bus.dtsi,
exynos5420-bus.dtsi, exynos5800-bus.dtsi which would include some
base file with the basic &bus_X and set the right parent, rate.
Then these files would be included into proper board file like:
exynos5800-peach-pi.dts.
Is this something that you would like to see?
Since the OPP tables
>>>>> are shared (they are property of the SoC, not board) then I would
>>>>> assume that default frequency is shared as well.
>>>> These clocks they all relay on some bootloader configuration. It depends
>>>> which version of the bootloader you have, then you might get different
>>>> default configuration in the clocks.
>>>
>>> I do not agree here. This configuration is not dependent on
>>> bootloader. Although one bootloader might set the clocks to X and
>>> other to Y, but still you provide here valid configuration setting
>>> them, e.g. to Y (or to Z). What bootloader set before does not matter
>>> because you always override it.
>> This exactly the patch set is aim to do: overwrite any bootloader
>> configuration which could be wrong set after boot.
>> I don't know for how long it is left in such
>> 'bootloader-default-clock-settings' but it is not accurate
>> configuration. The pattern in the DT to change the clock rates is
>> there.
>
> Still it is not the answer to my concerns and questions.
Please look at my answer above.
>
>>>
>>>> The pattern of changing the parent
>>>> or even rate is known in the DT files (or I am missing something).
>>>> When you grep for it, you get 168 hits (38 for exynos*):
>>>> git grep -n "assigned-clock-rates" ./arch/arm/boot/dts/ | wc -l
>>>
>>> Yeah, and if you grep per type you got:
>>> DTSI: 114
>>> DTS: 54
>>> so what do you want to say?
>> Thus, It could be changed in DT.
>
> Of course, why not. But how this relevant to my question?
Please see above.
>
>>> My thinking is that all the boards have buses configured to the same
>>> initial frequency. I am not questioning the use of
>>> assigned-clock-rates at all. Just the place...
>> It is not only 'initial frequency' as you name it. It has three changes:
>> - re-parent to proper PLL
>> - changing this PLL rate
>> - change the OPPs frequency values to integer values derived from PLL
>>
>> The initial frequencies will be changed by devfreq governor using OPP
>> tables and the load after the whole system boots.
>
> I simplified with "initial frequency" but it does not matter. Let me
> try to raise my concerns again, different wording:
> All this looks like property of the SoC, not the board, because:
> 1. the OPPs are already properties of the SoC, not the board (XU3 Lite
> is kind of exception but in fact it uses different flavor of
> Exynos5422 SoC which we do not model here as separate DTSI),
Please see above at (1).
> 2. I expect all boards to have the same properties.
All boards which have the same SoC, i.e. Exysno5422 <- then I agree.
Thank you for the comments.
Regards,
Lukasz
>
> Best regards,
> Krzysztof
>
>
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: i2c: Add MediaTek i2c AC timing binding
From: Qii Wang @ 2019-07-17 12:55 UTC (permalink / raw)
To: Wolfram Sang
Cc: devicetree, srv_heupstream, leilk.liu, linux-kernel, robh+dt,
linux-mediatek, linux-i2c, matthias.bgg, linux-arm-kernel
In-Reply-To: <20190626133941.GL801@ninjato>
On Wed, 2019-06-26 at 15:39 +0200, Wolfram Sang wrote:
> Hi,
>
> On Tue, Jun 11, 2019 at 04:11:54PM +0800, Qii Wang wrote:
> > Add i2c AC timing binding to binding file. It can give the AC
> > timing parameters to meet I2C specification at different speed.
> >
> > Signed-off-by: Qii Wang <qii.wang@mediatek.com>
>
> I think this is a too specific 1:1 mapping of your register set into DT
> world. We already have these generic parameters:
>
> - clock-frequency
> frequency of bus clock in Hz.
>
> - i2c-scl-falling-time-ns
> Number of nanoseconds the SCL signal takes to fall; t(f) in the I2C
> specification.
>
> - i2c-scl-internal-delay-ns
> Number of nanoseconds the IP core additionally needs to setup SCL.
>
> - i2c-scl-rising-time-ns
> Number of nanoseconds the SCL signal takes to rise; t(r) in the I2C
> specification.
>
> - i2c-sda-falling-time-ns
> Number of nanoseconds the SDA signal takes to fall; t(f) in the I2C
> specification.
>
> (check Documentation/devicetree/bindings/i2c/i2c.txt)
>
> Can't you use those to derive your values from that? Which ones are you missing
> if not?
>
> Regards,
>
> Wolfram
I have take a little time to develop a new patch which based on your
suggestions, and it tested OK. Thanks for your suggestions, I will
update the patch after I test it fully.
^ permalink raw reply
* Re: [PATCH v8 07/21] iommu/io-pgtable-arm-v7s: Extend MediaTek 4GB Mode
From: Yong Wu @ 2019-07-17 12:44 UTC (permalink / raw)
To: Will Deacon
Cc: youlin.pei-NuS5LvNUpcJWk0Htik3J/w,
devicetree-u79uwXL29TY76Z2rM5mHXA, Nicolas Boichat,
cui.zhang-NuS5LvNUpcJWk0Htik3J/w,
srv_heupstream-NuS5LvNUpcJWk0Htik3J/w, Tomasz Figa, Will Deacon,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Evan Green,
chao.hao-NuS5LvNUpcJWk0Htik3J/w,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, Rob Herring,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Matthias Brugger,
yingjoe.chen-NuS5LvNUpcJWk0Htik3J/w,
anan.sun-NuS5LvNUpcJWk0Htik3J/w, Robin Murphy, Matthias Kaehlcke,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <20190715095156.xczfkbm6zpjueq32@willie-the-truck>
On Mon, 2019-07-15 at 10:51 +0100, Will Deacon wrote:
> On Sun, Jul 14, 2019 at 12:41:20PM +0800, Yong Wu wrote:
> > On Thu, 2019-07-11 at 13:31 +0100, Will Deacon wrote:
> > > This looks like the right sort of idea. Basically, I was thinking that you
> > > can use the oas in conjunction with the quirk to specify whether or not
> > > your two magic bits should be set. You could also then cap the oas using
> > > the size of phys_addr_t to deal with my other comment.
> > >
> > > Finally, I was hoping you could drop the |= BIT_ULL(32) and the &=
> > > ~BIT_ULL(32) bits of the mtk driver if the pgtable code now accepts higher
> > > addresses. Did that not work out?
> >
> > After the current patch, the pgtable has accepted the higher address.
> > the " |= BIT_ULL(32)" and "& = ~ BIT_ULL(32)" is for a special case(we
> > call it 4GB mode).
> >
> > Now MediaTek IOMMU support 2 kind memory:
> > 1) normal case: PA is 0x4000_0000 - 0x3_ffff_ffff. the PA won't be
> > remapped. mt8183 and the non-4GB mode of mt8173/mt2712 use this mode.
> >
> > 2) 4GB Mode: PA is 0x4000_0000 - 0x1_3fff_ffff. But the PA will remapped
> > to 0x1_0000_0000 to 0x1_ffff_ffff. This is for the 4GB mode of
> > mt8173/mt2712. This case is so special that we should change the PA
> > manually(add bit32).
> > (mt2712 and mt8173 have both mode: 4GB and non-4GB.)
> >
> > If we try to use oas and our quirk to cover this two case. Then I can
> > use "oas == 33" only for this 4GB mode. and "oas == 34" for the normal
> > case even though the PA mayn't reach 34bit. Also I should add some
> > "workaround" for the 4GB mode(oas==33).
> >
> > I copy the new patch in the mail below(have dropped the "|= BIT_ULL(32)"
> > and the "&= ~BIT_ULL(32)) in mtk iommu". please help have a look if it
> > is ok.
> > (another thing: Current the PA can support over 4GB. So the quirk name
> > "MTK_4GB" looks not suitable, I used a new patch rename to "MTK_EXT").
>
> Makes sense, thanks. One comment below.
>
> > @@ -205,7 +216,20 @@ static phys_addr_t iopte_to_paddr(arm_v7s_iopte
> > pte, int lvl,
> > else
> > mask = ARM_V7S_LVL_MASK(lvl);
> >
> > - return pte & mask;
> > + paddr = pte & mask;
> > + if (IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT) &&
> > + (cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_EXT)) {
> > + /*
> > + * Workaround for MTK 4GB Mode:
> > + * Add BIT32 only when PA < 0x4000_0000.
> > + */
> > + if ((cfg->oas == 33 && paddr < 0x40000000UL) ||
> > + (cfg->oas > 33 && (pte & ARM_V7S_ATTR_MTK_PA_BIT32)))
> > + paddr |= BIT_ULL(32);
> > + if (pte & ARM_V7S_ATTR_MTK_PA_BIT33)
> > + paddr |= BIT_ULL(33);
> > + }
> > + return paddr;
> > }
> >
> > static arm_v7s_iopte *iopte_deref(arm_v7s_iopte pte, int lvl,
> > @@ -326,9 +350,6 @@ static arm_v7s_iopte arm_v7s_prot_to_pte(int prot,
> > int lvl,
> > if (lvl == 1 && (cfg->quirks & IO_PGTABLE_QUIRK_ARM_NS))
> > pte |= ARM_V7S_ATTR_NS_SECTION;
> >
> > - if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_EXT)
> > - pte |= ARM_V7S_ATTR_MTK_4GB;
> > -
> > return pte;
> > }
> >
> > @@ -742,7 +763,9 @@ static struct io_pgtable
> > *arm_v7s_alloc_pgtable(struct io_pgtable_cfg *cfg,
> > {
> > struct arm_v7s_io_pgtable *data;
> >
> > - if (cfg->ias > ARM_V7S_ADDR_BITS || cfg->oas > ARM_V7S_ADDR_BITS)
> > + if (cfg->ias > ARM_V7S_ADDR_BITS ||
> > + (cfg->oas > ARM_V7S_ADDR_BITS &&
> > + !(cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_EXT)))
> > return NULL;
>
> I think you can rework this to do something like:
>
> if (cfg->ias > ARM_V7S_ADDR_BITS)
> return NULL;
>
> if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_EXT) {
> if (!IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT))
> cfg->oas = min(cfg->oas, ARM_V7S_ADDR_BITS);
> else if (cfg->oas > 34)
> return NULL;
> } else if (cfg->oas > ARM_V7S_ADDR_BITS) {
> return NULL;
> }
>
> so that we clamp the oas when phys_addr_t is 32-bit for you. That should
> allow you to remove lots of the checking from iopte_to_paddr() too if you
> check against oas in the map() function.
>
> Does that make sense?
Of course I'm ok for this. I'm only afraid that this function has
already 3 checking "if (x) return NULL", Here we add a new one and so
many lines... Maybe the user should guarantee the right value of oas.
How about move it into mtk_iommu.c?
About the checking of iopte_to_paddr, I can not remove them. I know it
may be a bit special and not readable. Hmm, I guess I should use a MACRO
instead of the hard code 33 for the special 4GB mode case.
Then the patch would be like:
//=========================
static phys_addr_t iopte_to_paddr(arm_v7s_iopte pte, int lvl,
struct io_pgtable_cfg *cfg)
{
arm_v7s_iopte mask;
+ phys_addr_t paddr;
if (ARM_V7S_PTE_IS_TABLE(pte, lvl))
mask = ARM_V7S_TABLE_MASK;
@@ -205,7 +216,21 @@ static phys_addr_t iopte_to_paddr(arm_v7s_iopte
pte, int lvl,
else
mask = ARM_V7S_LVL_MASK(lvl);
- return pte & mask;
+ paddr = pte & mask;
+ if (cfg->oas > ARM_V7S_ADDR_BITS &&
+ (cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_EXT)) {
+ /*
+ * Workaround for MTK 4GB Mode:
+ * Add BIT32 only when PA < 0x4000_0000.
+ */
+ if (cfg->oas == ARM_V7S_MTK_4GB_OAS && paddr < 0x40000000UL)
+ paddr |= BIT_ULL(32);
+ else if (pte & ARM_V7S_ATTR_MTK_PA_BIT32)
+ paddr |= BIT_ULL(32);
+ if (pte & ARM_V7S_ATTR_MTK_PA_BIT33)
+ paddr |= BIT_ULL(33);
+ }
+ return paddr;
}
@@ -741,8 +763,10 @@ static struct io_pgtable
*arm_v7s_alloc_pgtable(struct io_pgtable_cfg *cfg,
void *cookie)
{
struct arm_v7s_io_pgtable *data;
-
- if (cfg->ias > ARM_V7S_ADDR_BITS || cfg->oas > ARM_V7S_ADDR_BITS)
+
+ if (cfg->ias > ARM_V7S_ADDR_BITS ||
+ (cfg->oas > ARM_V7S_ADDR_BITS &&
+ !(cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_EXT)))
return NULL;
if (cfg->quirks & ~(IO_PGTABLE_QUIRK_ARM_NS |
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 85e71fb..4efc5a3 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -271,16 +271,20 @@ static int mtk_iommu_domain_finalise(struct
mtk_iommu_domain *dom)
dom->cfg = (struct io_pgtable_cfg) {
.quirks = IO_PGTABLE_QUIRK_ARM_NS |
IO_PGTABLE_QUIRK_NO_PERMS |
- IO_PGTABLE_QUIRK_TLBI_ON_MAP,
+ IO_PGTABLE_QUIRK_TLBI_ON_MAP |
+ IO_PGTABLE_QUIRK_ARM_MTK_EXT,
.pgsize_bitmap = mtk_iommu_ops.pgsize_bitmap,
.ias = 32,
- .oas = 32,
.tlb = &mtk_iommu_gather_ops,
.iommu_dev = data->dev,
};
- if (data->enable_4GB)
- dom->cfg.quirks |= IO_PGTABLE_QUIRK_ARM_MTK_EXT;
+ if (!IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT))
+ dom->cfg.oas = 32;
+ else if (data->enable_4GB)
+ dom->cfg.oas = ARM_V7S_MTK_4GB_OAS;
+ else
+ dom->cfg.oas = 34;
--- a/include/linux/io-pgtable.h
+++ b/include/linux/io-pgtable.h
@@ -117,6 +116,8 @@ struct io_pgtable_cfg {
};
};
+#define ARM_V7S_MTK_4GB_OAS 33
+
/**
* struct io_pgtable_ops - Page table manipulation API for IOMMU
drivers.
*
=====================================
>
> Will
^ permalink raw reply related
* [PATCH 4/4][V2] dt-bindings: iio: imu: add bindings for ADIS16460
From: Alexandru Ardelean @ 2019-07-17 11:51 UTC (permalink / raw)
To: linux-iio, linux-spi, devicetree, linux-kernel
Cc: jic23, robh+dt, mark.rutland, broonie, Alexandru Ardelean
In-Reply-To: <20190717115109.15168-1-alexandru.ardelean@analog.com>
This change adds device-tree bindings for the ADIS16460.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
.../bindings/iio/imu/adi,adis16460.yaml | 53 +++++++++++++++++++
MAINTAINERS | 1 +
2 files changed, 54 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/imu/adi,adis16460.yaml
diff --git a/Documentation/devicetree/bindings/iio/imu/adi,adis16460.yaml b/Documentation/devicetree/bindings/iio/imu/adi,adis16460.yaml
new file mode 100644
index 000000000000..0c53009ba7d6
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/imu/adi,adis16460.yaml
@@ -0,0 +1,53 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/imu/adi,adis16460.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices ADIS16460 and similar IMUs
+
+maintainers:
+ - Dragos Bogdan <dragos.bogdan@analog.com>
+
+description: |
+ Analog Devices ADIS16460 and similar IMUs
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ADIS16460.pdf
+
+properties:
+ compatible:
+ enum:
+ - adi,adis16460
+
+ reg:
+ maxItems: 1
+
+ spi-cpha: true
+
+ spi-cpol: true
+
+ interrupts:
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+ - interrupts
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+ spi0 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ imu@0 {
+ compatible = "adi,adis16460";
+ reg = <0>;
+ spi-max-frequency = <5000000>;
+ spi-cpol;
+ spi-cpha;
+ interrupt-parent = <&gpio0>;
+ interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 8e679504c087..c44fbe8e91e9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -943,6 +943,7 @@ S: Supported
L: linux-iio@vger.kernel.org
W: http://ez.analog.com/community/linux-device-drivers
F: drivers/iio/imu/adis16460.c
+F: Documentation/devicetree/bindings/iio/imu/adi,adis16460.yaml
ANALOG DEVICES INC ADP5061 DRIVER
M: Stefan Popa <stefan.popa@analog.com>
--
2.20.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox