* [PATCH 3/5] arm64: dts: actions: Add gpio line names to Bubblegum-96 board
From: Manivannan Sadhasivam @ 2018-05-18 2:30 UTC (permalink / raw)
To: linus.walleij, robh+dt, afaerber
Cc: liuwei, mp-cs, 96boards, devicetree, andy.shevchenko,
daniel.thompson, amit.kucheria, linux-arm-kernel, linux-gpio,
linux-kernel, hzhang, bdong, manivannanece23,
Manivannan Sadhasivam
In-Reply-To: <20180518023056.7869-1-manivannan.sadhasivam@linaro.org>
Add gpio line names to Actions Semi S900 based Bubblegum-96 board.
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
arch/arm64/boot/dts/actions/s900-bubblegum-96.dts | 175 ++++++++++++++++++++++
1 file changed, 175 insertions(+)
diff --git a/arch/arm64/boot/dts/actions/s900-bubblegum-96.dts b/arch/arm64/boot/dts/actions/s900-bubblegum-96.dts
index ff043c961d75..d0ba35df9015 100644
--- a/arch/arm64/boot/dts/actions/s900-bubblegum-96.dts
+++ b/arch/arm64/boot/dts/actions/s900-bubblegum-96.dts
@@ -34,3 +34,178 @@
status = "okay";
clocks = <&cmu CLK_UART5>;
};
+
+/*
+ * GPIO name legend: proper name = the GPIO line is used as GPIO
+ * NC = not connected (pin out but not routed from the chip to
+ * anything the board)
+ * "[PER]" = pin is muxed for [peripheral] (not GPIO)
+ * LSEC = Low Speed External Connector
+ * HSEC = High Speed External Connector
+ *
+ * Line names are taken from the schematic "Schematics Bubblegum96"
+ * version v1.0
+ *
+ * For the lines routed to the external connectors the
+ * lines are named after the 96Boards CE Specification 1.0,
+ * Appendix "Expansion Connector Signal Description".
+ *
+ * When the 96Boards naming of a line and the schematic name of
+ * the same line are in conflict, the 96Boards specification
+ * takes precedence, which means that the external UART on the
+ * LSEC is named UART0 while the schematic and SoC names this
+ * UART2. Only exception is the I2C lines for which the schematic
+ * naming has been preferred. This is only for the informational
+ * lines i.e. "[FOO]", the GPIO named lines "GPIO-A" thru "GPIO-L"
+ * are the only ones actually used for GPIO.
+ */
+
+&pinctrl {
+ gpio-line-names =
+ "GPIO-A", /* GPIO_0, LSEC pin 23 */
+ "GPIO-B", /* GPIO_1, LSEC pin 24 */
+ "GPIO-C", /* GPIO_2, LSEC pin 25 */
+ "GPIO-D", /* GPIO_3, LSEC pin 26 */
+ "GPIO-E", /* GPIO_4, LSEC pin 27 */
+ "GPIO-F", /* GPIO_5, LSEC pin 28 */
+ "GPIO-G", /* GPIO_6, LSEC pin 29 */
+ "GPIO-H", /* GPIO_7, LSEC pin 30 */
+ "GPIO-I", /* GPIO_8, LSEC pin 31 */
+ "GPIO-J", /* GPIO_9, LSEC pin 32 */
+ "NC", /* GPIO_10 */
+ "NC", /* GPIO_11 */
+ "SIRQ2_1V8", /* GPIO_12 */
+ "PCM0_OUT", /* GPIO_13 */
+ "WIFI_LED", /* GPIO_14 */
+ "PCM0_SYNC", /* GPIO_15 */
+ "PCM0_CLK", /* GPIO_16 */
+ "PCM0_IN", /* GPIO_17 */
+ "BT_LED", /* GPIO_18 */
+ "LED0", /* GPIO_19 */
+ "LED1", /* GPIO_20 */
+ "JTAG_TCK", /* GPIO_21 */
+ "JTAG_TMS", /* GPIO_22 */
+ "JTAG_TDI", /* GPIO_23 */
+ "JTAG_TDO", /* GPIO_24 */
+ "[UART1_RxD]", /* GPIO_25, LSEC pin 13 */
+ "NC", /* GPIO_26 */
+ "[UART1_TxD]", /* GPIO_27, LSEC pin 11 */
+ "SD0_D0", /* GPIO_28 */
+ "SD0_D1", /* GPIO_29 */
+ "SD0_D2", /* GPIO_30 */
+ "SD0_D3", /* GPIO_31 */
+ "SD1_D0", /* GPIO_32 */
+ "SD1_D1", /* GPIO_33 */
+ "SD1_D2", /* GPIO_34 */
+ "SD1_D3", /* GPIO_35 */
+ "SD0_CMD", /* GPIO_36 */
+ "SD0_CLK", /* GPIO_37 */
+ "SD1_CMD", /* GPIO_38 */
+ "SD1_CLK", /* GPIO_39 */
+ "SPI0_SCLK", /* GPIO_40, LSEC pin 8 */
+ "SPI0_CS", /* GPIO_41, LSEC pin 12 */
+ "SPI0_DIN", /* GPIO_42, LSEC pin 10 */
+ "SPI0_DOUT", /* GPIO_43, LSEC pin 14 */
+ "I2C5_SDATA", /* GPIO_44, HSEC pin 36 */
+ "I2C5_SCLK", /* GPIO_45, HSEC pin 38 */
+ "UART0_RX", /* GPIO_46, LSEC pin 7 */
+ "UART0_TX", /* GPIO_47, LSEC pin 5 */
+ "UART0_RTSB", /* GPIO_48, LSEC pin 9 */
+ "UART0_CTSB", /* GPIO_49, LSEC pin 3 */
+ "I2C4_SCLK", /* GPIO_50, HSEC pin 32 */
+ "I2C4_SDATA", /* GPIO_51, HSEC pin 34 */
+ "I2C0_SCLK", /* GPIO_52 */
+ "I2C0_SDATA", /* GPIO_53 */
+ "I2C1_SCLK", /* GPIO_54, LSEC pin 15 */
+ "I2C1_SDATA", /* GPIO_55, LSEC pin 17 */
+ "I2C2_SCLK", /* GPIO_56, LSEC pin 19 */
+ "I2C2_SDATA", /* GPIO_57, LSEC pin 21 */
+ "CSI0_DN0", /* GPIO_58, HSEC pin 10 */
+ "CSI0_DP0", /* GPIO_59, HSEC pin 8 */
+ "CSI0_DN1", /* GPIO_60, HSEC pin 16 */
+ "CSI0_DP1", /* GPIO_61, HSEC pin 14 */
+ "CSI0_CN", /* GPIO_62, HSEC pin 4 */
+ "CSI0_CP", /* GPIO_63, HSEC pin 2 */
+ "CSI0_DN2", /* GPIO_64, HSEC pin 22 */
+ "CSI0_DP2", /* GPIO_65, HSEC pin 20 */
+ "CSI0_DN3", /* GPIO_66, HSEC pin 28 */
+ "CSI0_DP3", /* GPIO_67, HSEC pin 26 */
+ "[CLK0]", /* GPIO_68, HSEC pin 15 */
+ "CSI1_DN0", /* GPIO_69, HSEC pin 44 */
+ "CSI1_DP0", /* GPIO_70, HSEC pin 42 */
+ "CSI1_DN1", /* GPIO_71, HSEC pin 50 */
+ "CSI1_DP1", /* GPIO_72, HSEC pin 48 */
+ "CSI1_CN", /* GPIO_73, HSEC pin 56 */
+ "CSI1_CP", /* GPIO_74, HSEC pin 54 */
+ "[CLK1]", /* GPIO_75, HSEC pin 17 */
+ "[GPIOD0]", /* GPIO_76 */
+ "[GPIOD1]", /* GPIO_77 */
+ "BT_RST_N", /* GPIO_78 */
+ "EXT_DC_EN", /* GPIO_79 */
+ "[PCM_DI]", /* GPIO_80, LSEC pin 22 */
+ "[PCM_DO]", /* GPIO_81, LSEC pin 20 */
+ "[PCM_CLK]", /* GPIO_82, LSEC pin 18 */
+ "[PCM_FS]", /* GPIO_83, LSEC pin 16 */
+ "WAKE_BT", /* GPIO_84 */
+ "WL_REG_ON", /* GPIO_85 */
+ "NC", /* GPIO_86 */
+ "NC", /* GPIO_87 */
+ "NC", /* GPIO_88 */
+ "NC", /* GPIO_89 */
+ "NC", /* GPIO_90 */
+ "WIFI_WAKE", /* GPIO_91 */
+ "BT_WAKE", /* GPIO_92 */
+ "NC", /* GPIO_93 */
+ "OTG_EN2", /* GPIO_94 */
+ "OTG_EN", /* GPIO_95 */
+ "DSI_DP3", /* GPIO_96, HSEC pin 45 */
+ "DSI_DN3", /* GPIO_97, HSEC pin 47 */
+ "DSI_DP1", /* GPIO_98, HSEC pin 33 */
+ "DSI_DN1", /* GPIO_99, HSEC pin 35 */
+ "DSI_CP", /* GPIO_100, HSEC pin 21 */
+ "DSI_CN", /* GPIO_101, HSEC pin 23 */
+ "DSI_DP0", /* GPIO_102, HSEC pin 27 */
+ "DSI_DN0", /* GPIO_103, HSEC pin 29 */
+ "DSI_DP2", /* GPIO_104, HSEC pin 39 */
+ "DSI_DN2", /* GPIO_105, HSEC pin 41 */
+ "N0_D0", /* GPIO_106 */
+ "N0_D1", /* GPIO_107 */
+ "N0_D2", /* GPIO_108 */
+ "N0_D3", /* GPIO_109 */
+ "N0_D4", /* GPIO_110 */
+ "N0_D5", /* GPIO_111 */
+ "N0_D6", /* GPIO_112 */
+ "N0_D7", /* GPIO_113 */
+ "N0_DQS", /* GPIO_114 */
+ "N0_DQSN", /* GPIO_115 */
+ "NC", /* GPIO_116 */
+ "NC", /* GPIO_117 */
+ "NC", /* GPIO_118 */
+ "N0_CEB1", /* GPIO_119 */
+ "CARD_DT", /* GPIO_120 */
+ "N0_CEB3", /* GPIO_121 */
+ "SD_DAT0", /* GPIO_122, HSEC pin 1 */
+ "SD_DAT1", /* GPIO_123, HSEC pin 3 */
+ "SD_DAT2", /* GPIO_124, HSEC pin 5 */
+ "SD_DAT3", /* GPIO_125, HSEC pin 7 */
+ "NC", /* GPIO_126 */
+ "NC", /* GPIO_127 */
+ "[PWR_BTN_N]", /* GPIO_128, LSEC pin 4 */
+ "[RST_BTN_N]", /* GPIO_129, LSEC pin 6 */
+ "NC", /* GPIO_130 */
+ "SD_CMD", /* GPIO_131 */
+ "GPIO-L", /* GPIO_132, LSEC pin 34 */
+ "GPIO-K", /* GPIO_133, LSEC pin 33 */
+ "NC", /* GPIO_134 */
+ "SD_SCLK", /* GPIO_135 */
+ "NC", /* GPIO_136 */
+ "JTAG_TRST", /* GPIO_137 */
+ "I2C3_SCLK", /* GPIO_138 */
+ "LED2", /* GPIO_139 */
+ "LED3", /* GPIO_140 */
+ "I2C3_SDATA", /* GPIO_141 */
+ "UART3_RX", /* GPIO_142 */
+ "UART3_TX", /* GPIO_143 */
+ "UART3_RTSB", /* GPIO_144 */
+ "UART3_CTSB"; /* GPIO_145 */
+};
--
2.14.1
^ permalink raw reply related
* [PATCH 2/5] arm64: dts: actions: Add gpio properties to pinctrl node for S900
From: Manivannan Sadhasivam @ 2018-05-18 2:30 UTC (permalink / raw)
To: linus.walleij, robh+dt, afaerber
Cc: liuwei, mp-cs, 96boards, devicetree, andy.shevchenko,
daniel.thompson, amit.kucheria, linux-arm-kernel, linux-gpio,
linux-kernel, hzhang, bdong, manivannanece23,
Manivannan Sadhasivam
In-Reply-To: <20180518023056.7869-1-manivannan.sadhasivam@linaro.org>
Add gpio properties to pinctrl node for Actions Semi S900 SoC.
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
arch/arm64/boot/dts/actions/s900.dtsi | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm64/boot/dts/actions/s900.dtsi b/arch/arm64/boot/dts/actions/s900.dtsi
index 0156483f0f4d..05f31a954733 100644
--- a/arch/arm64/boot/dts/actions/s900.dtsi
+++ b/arch/arm64/boot/dts/actions/s900.dtsi
@@ -178,6 +178,8 @@
compatible = "actions,s900-pinctrl";
reg = <0x0 0xe01b0000 0x0 0x1000>;
clocks = <&cmu CLK_GPIO>;
+ gpio-controller;
+ #gpio-cells = <2>;
};
timer: timer@e0228000 {
--
2.14.1
^ permalink raw reply related
* [PATCH 1/5] dt-bindings: pinctrl: Add gpio bindings for Actions S900 SoC
From: Manivannan Sadhasivam @ 2018-05-18 2:30 UTC (permalink / raw)
To: linus.walleij, robh+dt, afaerber
Cc: liuwei, mp-cs, 96boards, devicetree, andy.shevchenko,
daniel.thompson, amit.kucheria, linux-arm-kernel, linux-gpio,
linux-kernel, hzhang, bdong, manivannanece23,
Manivannan Sadhasivam
In-Reply-To: <20180518023056.7869-1-manivannan.sadhasivam@linaro.org>
Add gpio bindings for Actions Semi S900 SoC.
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
.../devicetree/bindings/pinctrl/actions,s900-pinctrl.txt | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/Documentation/devicetree/bindings/pinctrl/actions,s900-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/actions,s900-pinctrl.txt
index fb87c7d74f2e..300a50783aab 100644
--- a/Documentation/devicetree/bindings/pinctrl/actions,s900-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/actions,s900-pinctrl.txt
@@ -8,6 +8,15 @@ Required Properties:
- reg: Should contain the register base address and size of
the pin controller.
- clocks: phandle of the clock feeding the pin controller
+- gpio-controller: Marks the device node as a GPIO controller.
+- #gpio-cells: Should be two. The first cell is the gpio pin number
+ and the second cell is used for optional parameters.
+- interrupt-controller: Marks the device node as an interrupt controller.
+- #interrupt-cells: Specifies the number of cells needed to encode an
+ interrupt. Shall be set to 2. The first cell
+ defines the interrupt number, the second encodes
+ the trigger flags described in
+ bindings/interrupt-controller/interrupts.txt
Please refer to pinctrl-bindings.txt in this directory for details of the
common pinctrl bindings used by client devices, including the meaning of the
@@ -164,6 +173,10 @@ Example:
compatible = "actions,s900-pinctrl";
reg = <0x0 0xe01b0000 0x0 0x1000>;
clocks = <&cmu CLK_GPIO>;
+ gpio-controller;
+ #gpio-cells = <2>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
uart2-default: uart2-default {
pinmux {
--
2.14.1
^ permalink raw reply related
* [PATCH 0/5] Add gpio support for Action Semi S900 SoC
From: Manivannan Sadhasivam @ 2018-05-18 2:30 UTC (permalink / raw)
To: linus.walleij, robh+dt, afaerber
Cc: liuwei, mp-cs, 96boards, devicetree, andy.shevchenko,
daniel.thompson, amit.kucheria, linux-arm-kernel, linux-gpio,
linux-kernel, hzhang, bdong, manivannanece23,
Manivannan Sadhasivam
This patchset adds gpio support for Actions Semi S900 SoC by extending
the pinctrl driver. There were previous patches submitted for adding a
standalone gpio driver based on gpiolib. But later on it has been realised
that the gpio functionality is closely tied with pinctrl subsystem for this
OWL family processors. So, having a separate gpio driver will make it hard
to add further functionalities in future. Hence, we decided to drop the
previous patches below adding a standalone gpio support:
dt-bindings: gpio: Add gpio nodes for Actions S900 SoC
arm64: dts: actions: Add S900 gpio nodes
arm64: dts: actions: Add gpio line names to Bubblegum-96 board
gpio: Add gpio driver for Actions OWL S900 SoC
MAINTAINERS: Add Actions Semi S900 pinctrl and gpio entries
This patchset consits of incremental patches which will apply with the
previous pinctrl series: Add Actions Semi S900 pinctrl and gpio support,
excluding the dropped patches mentioned above.
Thanks,
Mani
Manivannan Sadhasivam (5):
dt-bindings: pinctrl: Add gpio bindings for Actions S900 SoC
arm64: dts: actions: Add gpio properties to pinctrl node for S900
arm64: dts: actions: Add gpio line names to Bubblegum-96 board
pinctrl: actions: Add gpio support for Actions S900 SoC
MAINTAINERS: Add Actions Semi S900 pinctrl entries
.../bindings/pinctrl/actions,s900-pinctrl.txt | 13 ++
MAINTAINERS | 2 +
arch/arm64/boot/dts/actions/s900-bubblegum-96.dts | 175 +++++++++++++++++
arch/arm64/boot/dts/actions/s900.dtsi | 2 +
drivers/pinctrl/actions/Kconfig | 1 +
drivers/pinctrl/actions/pinctrl-owl.c | 206 +++++++++++++++++++++
drivers/pinctrl/actions/pinctrl-owl.h | 20 ++
drivers/pinctrl/actions/pinctrl-s900.c | 29 ++-
8 files changed, 447 insertions(+), 1 deletion(-)
--
2.14.1
^ permalink raw reply
* Re: [PATCH] PM / devfreq: Add support for QCOM devfreq FW
From: Chanwoo Choi @ 2018-05-18 2:28 UTC (permalink / raw)
To: Saravana Kannan, MyungJoo Ham, Kyungmin Park, Rob Herring,
Mark Rutland
Cc: linux-pm, devicetree, linux-kernel
In-Reply-To: <1526536958-29419-1-git-send-email-skannan@codeaurora.org>
Hi,
On 2018년 05월 17일 15:02, Saravana Kannan wrote:
> The firmware present in some QCOM chipsets offloads the steps necessary for
> changing the frequency of some devices (Eg: L3). This driver implements the
> devfreq interface for this firmware so that various governors could be used
> to scale the frequency of these devices.
The description doesn't include what kind of firmware. You have to explain
the type and role of firmware. And it doesn't contain the description of correlation
between 'qcom,devfreq-fw' and 'qcom,devfreq-fw-voter'.
>
> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> ---
> .../bindings/devfreq/devfreq-qcom-fw.txt | 31 ++
> drivers/devfreq/Kconfig | 14 +
> drivers/devfreq/Makefile | 1 +
> drivers/devfreq/devfreq_qcom_fw.c | 326 +++++++++++++++++++++
> 4 files changed, 372 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/devfreq/devfreq-qcom-fw.txt
> create mode 100644 drivers/devfreq/devfreq_qcom_fw.c
>
> diff --git a/Documentation/devicetree/bindings/devfreq/devfreq-qcom-fw.txt b/Documentation/devicetree/bindings/devfreq/devfreq-qcom-fw.txt
> new file mode 100644
> index 0000000..5e1aecf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/devfreq/devfreq-qcom-fw.txt
> @@ -0,0 +1,31 @@
> +QCOM Devfreq FW device
> +
> +Some Qualcomm Technologies, Inc. (QTI) chipsets have a FW that offloads the
you better to change the FW for more readability.
- s/FW/firmware
> +steps for frequency switching. The qcom,devfreq-fw represents this FW as a
- s/FW/firmware
Usually, DVFS feature uses the 'frequency scaling' word.
- s/frequency switching/frequency scaling
> +device. Sometimes, multiple entities want to vote on the frequency request
> +that is sent to the FW. The qcom,devfreq-fw-voter represents these voters as
s/FW/firmware
> +child devices of the corresponding qcom,devfreq-fw device.
> +
> +Required properties:
> +- compatible: Must be "qcom,devfreq-fw" or "qcom,devfreq-fw-voter"
The use of 'devfreq' word is not proper because 'devfreq' is framework name.
I think you have to use the specific SoC name for the compatible. In the future,
if you need to support the new SoC with this driver, just you can add the new compatible.
> +Only for qcom,devfreq-fw:
> +- reg: Pairs of physical base addresses and region sizes of
> + memory mapped registers.
> +- reg-names: Names of the bases for the above registers. Expected
> + bases are: "en-base", "lut-base" and "perf-base".
It is not possible to understand what are meaning of "en-base", "lut-base" and "perf-base".
because they depend on specific H/W specification. You have to add the detailed description
why they are necessary. Also, you should explain whether thery are mandatory or optional.
> +
> +Example:
> +
> + qcom,devfreq-l3 {
> + compatible = "qcom,devfreq-fw";
> + reg-names = "en-base", "lut-base", "perf-base";
> + reg = <0x18321000 0x4>, <0x18321110 0x600>, <0x18321920 0x4>;
> +
> + qcom,cpu0-l3 {
> + compatible = "qcom,devfreq-fw-voter";
> + };
> +
> + qcom,cpu4-l3 {
> + compatible = "qcom,devfreq-fw-voter";
> + };
> + };
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 6a172d3..8503018 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -113,6 +113,20 @@ config ARM_RK3399_DMC_DEVFREQ
> It sets the frequency for the memory controller and reads the usage counts
> from hardware.
>
> +config ARM_QCOM_DEVFREQ_FW
> + bool "Qualcomm Technologies Inc. DEVFREQ FW driver"
> + depends on ARCH_QCOM
> + select DEVFREQ_GOV_PERFORMANCE
> + select DEVFREQ_GOV_POWERSAVE
> + select DEVFREQ_GOV_USERSPACE
> + default n
> + help
> + The firmware present in some QCOM chipsets offloads the steps
> + necessary for changing the frequency of some devices (Eg: L3). This
> + driver implements the devfreq interface for this firmware so that
> + various governors could be used to scale the frequency of these
> + devices.
As I commented, you need to add a description of what kind of firmwar.
> +
> source "drivers/devfreq/event/Kconfig"
>
> endif # PM_DEVFREQ
> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> index 32b8d4d..f1cc8990 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE) += governor_passive.o
> obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ) += exynos-bus.o
> obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ) += rk3399_dmc.o
> obj-$(CONFIG_ARM_TEGRA_DEVFREQ) += tegra-devfreq.o
> +obj-$(CONFIG_ARM_QCOM_DEVFREQ_FW) += devfreq_qcom_fw.o
>
> # DEVFREQ Event Drivers
> obj-$(CONFIG_PM_DEVFREQ_EVENT) += event/
> diff --git a/drivers/devfreq/devfreq_qcom_fw.c b/drivers/devfreq/devfreq_qcom_fw.c
> new file mode 100644
> index 0000000..3e85f76
> --- /dev/null
> +++ b/drivers/devfreq/devfreq_qcom_fw.c
> @@ -0,0 +1,326 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
Is it right? or Qualcomm?
> + */
> +
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/list.h>
> +#include <linux/devfreq.h>
> +#include <linux/pm_opp.h>
> +
> +#define INIT_RATE 300000000UL
> +#define XO_RATE 19200000UL
> +#define LUT_MAX_ENTRIES 40U
> +#define LUT_ROW_SIZE 32
I don't know what are the meaning of 'XO', 'LUT'.
> +
> +struct devfreq_qcom_fw {
> + void __iomem *perf_base;
> + struct devfreq_dev_profile dp;
> + struct list_head ;
> + struct list_head voter;
> + unsigned int index;
> +};
> +
> +static DEFINE_SPINLOCK(voter_lock);
> +
> +static int devfreq_qcom_fw_target(struct device *dev, unsigned long *freq,
> + u32 flags)
> +{
> + struct devfreq_qcom_fw *d = dev_get_drvdata(dev), *pd, *v;
> + struct devfreq_dev_profile *p = &d->dp;
> + unsigned int index;
> + unsigned long lflags;
> + struct dev_pm_opp *opp;
> + void __iomem *perf_base = d->perf_base;
> +
> + opp = devfreq_recommended_opp(dev, freq, flags);
> + if (!IS_ERR(opp))
> + dev_pm_opp_put(opp);
> + else
> + return PTR_ERR(opp);
> +
> + for (index = 0; index < p->max_state; index++)
> + if (p->freq_table[index] == *freq)
> + break;
> +
> + if (index >= p->max_state) {
> + dev_err(dev, "Unable to find index for freq (%lu)!\n", *freq);
> + return -EINVAL;
> + }
> +
> + d->index = index;
> +
> + spin_lock_irqsave(&voter_lock, lflags);
> + /* Voter */
> + if (!perf_base) {
> + pd = dev_get_drvdata(dev->parent);
> + list_for_each_entry(v, &pd->voters, voter)
> + index = max(index, v->index);
> + perf_base = pd->perf_base;
> + }
> +
> + writel_relaxed(index, perf_base);
> + spin_unlock_irqrestore(&voter_lock, lflags);
> +
> + return 0;
> +}
> +
> +static int devfreq_qcom_fw_get_cur_freq(struct device *dev,
> + unsigned long *freq)
> +{
> + struct devfreq_qcom_fw *d = dev_get_drvdata(dev);
> + struct devfreq_dev_profile *p = &d->dp;
> + unsigned int index;
> +
> + /* Voter */
> + if (!d->perf_base) {
> + index = d->index;
> + } else {
> + index = readl_relaxed(d->perf_base);
> + index = min(index, p->max_state - 1);
> + }
> + *freq = p->freq_table[index];
> +
> + return 0;
> +}
> +
> +static int devfreq_qcom_populate_opp(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + u32 data, src, lval, i;
> + unsigned long freq, prev_freq;
> + struct resource *res;
> + void __iomem *lut_base;
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "lut-base");
> + if (!res) {
> + dev_err(dev, "Unable to find lut-base!\n");
> + return -EINVAL;
> + }
> +
> + lut_base = devm_ioremap(dev, res->start, resource_size(res));
> + if (!lut_base) {
> + dev_err(dev, "Unable to map lut-base\n");
> + return -ENOMEM;
> + }
> +
> + for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> + data = readl_relaxed(lut_base + i * LUT_ROW_SIZE);
> + src = ((data & GENMASK(31, 30)) >> 30);
> + lval = (data & GENMASK(7, 0));
> + freq = src ? XO_RATE * lval : INIT_RATE;
You need to define the global definitions such as XXX_MASK, XXX_SHIFT
for the readability. And add the role of 'lval' and why have to multiply
'lavl' to 'XO_RATE'.
> +
> + /*
> + * Two of the same frequencies with the same core counts means
> + * end of table.
> + */
> + if (i > 0 && prev_freq == freq)
> + break;
> +
> + dev_pm_opp_add(&pdev->dev, freq, 0);
> +
> + prev_freq = freq;
> + }
> +
> + devm_iounmap(dev, lut_base);
> +
> + return 0;
> +}
> +
> +static int devfreq_qcom_init_hw(struct platform_device *pdev)
> +{
> + struct devfreq_qcom_fw *d;
> + struct resource *res;
> + struct device *dev = &pdev->dev;
> + int ret = 0;
> + void __iomem *en_base;
> +
> + d = devm_kzalloc(dev, sizeof(*d), GFP_KERNEL);
> + if (!d)
> + return -ENOMEM;
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "en-base");
> + if (!res) {
> + dev_err(dev, "Unable to find en-base!\n");
> + return -EINVAL;
> + }
> +
> + en_base = devm_ioremap(dev, res->start, resource_size(res));
> + if (!en_base) {
> + dev_err(dev, "Unable to map en-base\n");
> + return -ENOMEM;
> + }
> +
> + /* FW should be enabled state to proceed */
> + if (!(readl_relaxed(en_base) & 1)) {
> + dev_err(dev, "FW not enabled\n");
> + return -ENODEV;
> + }
> +
> + devm_iounmap(dev, en_base);
> +
> + ret = devfreq_qcom_populate_opp(pdev);
> + if (ret) {
> + dev_err(dev, "Failed to read LUT\n");
> + return ret;
> + }
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "perf-base");
> + if (!res) {
> + dev_err(dev, "Unable to find perf-base!\n");
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + d->perf_base = devm_ioremap(dev, res->start, resource_size(res));
> + if (!d->perf_base) {
> + dev_err(dev, "Unable to map perf-base\n");
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + INIT_LIST_HEAD(&d->voters);
> + dev_set_drvdata(dev, d);
> +
> +out:
> + if (ret)
> + dev_pm_opp_remove_table(dev);
> + return ret;
> +}
> +
> +static int devfreq_qcom_copy_opp(struct device *src_dev, struct device *dst_dev)
> +{
> + unsigned long freq;
> + int i, cnt, ret = 0;
> + struct dev_pm_opp *opp;
> +
> + if (!src_dev)
> + return -ENODEV;
> +
> + cnt = dev_pm_opp_get_opp_count(src_dev);
> + if (!cnt)
> + return -EINVAL;
> +
> + for (i = 0, freq = 0; i < cnt; i++, freq++) {
> + opp = dev_pm_opp_find_freq_ceil(src_dev, &freq);
> + if (IS_ERR(opp)) {
> + ret = -EINVAL;
> + break;
> + }
> + dev_pm_opp_put(opp);
> +
> + ret = dev_pm_opp_add(dst_dev, freq, 0);
> + if (ret)
> + break;
> + }
> +
> + if (ret)
> + dev_pm_opp_remove_table(dst_dev);
> + return ret;
> +}
> +
> +static int devfreq_qcom_init_voter(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device *par_dev = dev->parent;
> + struct devfreq_qcom_fw *d, *pd = dev_get_drvdata(par_dev);
> + int ret = 0;
> +
> + d = devm_kzalloc(dev, sizeof(*d), GFP_KERNEL);
> + if (!d)
> + return -ENOMEM;
> +
> + ret = devfreq_qcom_copy_opp(dev->parent, dev);
> + if (ret) {
> + dev_err(dev, "Failed to copy parent OPPs\n");
> + return ret;
> + }
> +
> + list_add(&d->voter, &pd->voters);
> + dev_set_drvdata(dev, d);
> +
> + return 0;
> +}
> +
> +static int devfreq_qcom_fw_driver_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + int ret = 0;
> + struct devfreq_qcom_fw *d;
> + struct devfreq_dev_profile *p;
> + struct devfreq *df;
> +
> + if (!of_device_get_match_data(dev))
> + ret = devfreq_qcom_init_voter(pdev);
> + else
> + ret = devfreq_qcom_init_hw(pdev);
> + if (ret) {
> + dev_err(dev, "Unable to probe device!\n");
> + return ret;
> + }
> +
> + /*
> + * If device has voter children, do no register directly with devfreq
> + */
> + if (of_get_available_child_count(dev->of_node)) {
> + of_platform_populate(dev->of_node, NULL, NULL, dev);
> + dev_info(dev, "Devfreq QCOM FW parent device initialized.\n");
> + return 0;
> + }
> +
> + d = dev_get_drvdata(dev);
> + p = &d->dp;
> + p->polling_ms = 50;
> + p->target = devfreq_qcom_fw_target;
> + p->get_cur_freq = devfreq_qcom_fw_get_cur_freq;
> +
> + df = devm_devfreq_add_device(dev, p, "performance", NULL);
> + if (IS_ERR(df)) {
> + dev_err(dev, "Unable to register Devfreq QCOM FW device!\n");
> + return PTR_ERR(df);
> + }
> +
> + dev_info(dev, "Devfreq QCOM FW device registered.\n");
> +
> + return 0;
> +}
> +
> +static const struct of_device_id match_table[] = {
> + { .compatible = "qcom,devfreq-fw", .data = (void *) 1 },
> + { .compatible = "qcom,devfreq-fw-voter", .data = (void *) 0 },
> + {}
> +};
> +
> +static struct platform_driver devfreq_qcom_fw_driver = {
> + .probe = devfreq_qcom_fw_driver_probe,
> + .driver = {
> + .name = "devfreq-qcom-fw",
> + .of_match_table = match_table,
> + .owner = THIS_MODULE,
> + },
> +};
> +
> +static int __init devfreq_qcom_fw_init(void)
> +{
> + return platform_driver_register(&devfreq_qcom_fw_driver);
> +}
> +subsys_initcall(devfreq_qcom_fw_init);
> +
> +static void __exit devfreq_qcom_fw_exit(void)
> +{
> + platform_driver_unregister(&devfreq_qcom_fw_driver);
> +}
> +module_exit(devfreq_qcom_fw_exit);
> +
> +MODULE_DESCRIPTION("Devfreq QCOM FW");
> +MODULE_LICENSE("GPL v2");
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH v5 4/4] drm/rockchip: support dp training outside dp firmware
From: Brian Norris @ 2018-05-18 1:45 UTC (permalink / raw)
To: hl
Cc: devicetree, open list:ARM/Rockchip SoC..., David Airlie,
Doug Anderson, Linux Kernel, Rob Herring, dri-devel, Chris Zhong,
Daniel Vetter, Kishon Vijay Abraham I, linux-arm-kernel
In-Reply-To: <d4c38236-7455-ccd5-a1d9-a000b7d78be9@rock-chips.com>
On Thu, May 17, 2018 at 6:41 PM, hl <hl@rock-chips.com> wrote:
> On Thursday, May 17, 2018 09:51 PM, Sean Paul wrote:
>> On Thu, May 17, 2018 at 05:18:00PM +0800, Lin Huang wrote:
>>> DP firmware uses fixed phy config values to do training, but some
>>> boards need to adjust these values to fit for their unique hardware
>>> design. So get phy config values from dts and use software link training
>>> instead of relying on firmware, if software training fail, keep firmware
>>> training as a fallback if sw training fails.
>>>
>>> Signed-off-by: Chris Zhong <zyw@rock-chips.com>
>>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>>> ---
>>> Changes in v2:
>>> - update patch following Enric suggest
>>> Changes in v3:
>>> - use variable fw_training instead sw_training_success
>>> - base on DP SPCE, if training fail use lower link rate to retry training
>>> Changes in v4:
>>> - improve cdn_dp_get_lower_link_rate() and cdn_dp_software_train_link() follow Sean suggest
>>> Changes in v5:
>>> - fix some whitespcae issue
>>>
>>> drivers/gpu/drm/rockchip/Makefile | 3 +-
>>> drivers/gpu/drm/rockchip/cdn-dp-core.c | 24 +-
>>> drivers/gpu/drm/rockchip/cdn-dp-core.h | 2 +
>>> drivers/gpu/drm/rockchip/cdn-dp-link-training.c | 420 ++++++++++++++++++++++++
>>> drivers/gpu/drm/rockchip/cdn-dp-reg.c | 31 +-
>>> drivers/gpu/drm/rockchip/cdn-dp-reg.h | 38 ++-
>>> 6 files changed, 505 insertions(+), 13 deletions(-)
>>> create mode 100644 drivers/gpu/drm/rockchip/cdn-dp-link-training.c
>>>
...
>>> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-link-training.c b/drivers/gpu/drm/rockchip/cdn-dp-link-training.c
>>> new file mode 100644
>>> index 0000000..73c3290
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/rockchip/cdn-dp-link-training.c
>>> @@ -0,0 +1,420 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
>>> + * Author: Chris Zhong <zyw@rock-chips.com>
>>> + */
>>> +
>>> +#include <linux/device.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/phy/phy.h>
>>> +#include <soc/rockchip/rockchip_phy_typec.h>
>>> +
>>> +#include "cdn-dp-core.h"
>>> +#include "cdn-dp-reg.h"
>>> +
>>> +static void cdn_dp_set_signal_levels(struct cdn_dp_device *dp)
>>> +{
>>> + struct cdn_dp_port *port = dp->port[dp->active_port];
>>> + struct rockchip_typec_phy *tcphy = phy_get_drvdata(port->phy);
>>
>> You ignored Brian's comment on the previous patch:
>> This is still antithetical to the PHY framework; you're assuming that
>> this is a particular type of PHY here.
>>
>> FWIW, the mediatek drm driver also assumes a certain PHY type. A quick grep of
>> drivers/ shows that the only other non-phy/ driver using this function
>> (pinctrl-tegra-xusb.c) also casts it.
>>
>> Sean
>
> Thanks Sean, except phy framework have new API to handle it, i have not
> idea how to do it in a better way.
Well, if Mediatek can do it for their MIPI and HDMI, then maybe we just do it...
Brian
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v8 10/15] cpufreq: Add Kryo CPU scaling driver
From: Viresh Kumar @ 2018-05-18 1:45 UTC (permalink / raw)
To: Ilia Lin
Cc: mturquette, sboyd, robh, mark.rutland, nm, lgirdwood, broonie,
andy.gross, david.brown, catalin.marinas, will.deacon, rjw,
linux-clk, devicetree, linux-kernel, linux-pm, linux-arm-msm,
linux-soc, linux-arm-kernel, rnayak, amit.kucheria,
nicolas.dechesne, celster, tfinkel
In-Reply-To: <1526555955-29960-11-git-send-email-ilialin@codeaurora.org>
On 17-05-18, 14:19, Ilia Lin wrote:
> +static int __init qcom_cpufreq_kryo_driver_init(void)
> +{
> + size_t len;
> + int ret = 0;
> + u32 versions;
> + enum _msm8996_version msm8996_version;
> + u8 *speedbin;
> + struct device *cpu_dev_silver, *cpu_dev_gold;
> + struct device_node *np;
> + struct nvmem_cell *speedbin_nvmem;
> + struct platform_device *pdev;
> + struct opp_table *opp_silver = NULL;
> + struct opp_table *opp_gold = NULL;
No need to initialize them and you may want to arrange all above in
decreasing order of their length.
> +
> + cpu_dev_silver = get_cpu_device(SILVER_LEAD);
> + if (IS_ERR_OR_NULL(cpu_dev_silver))
> + return PTR_ERR(cpu_dev_silver);
> +
> + cpu_dev_gold = get_cpu_device(SILVER_LEAD);
> + if (IS_ERR_OR_NULL(cpu_dev_gold))
> + return PTR_ERR(cpu_dev_gold);
> +
> + msm8996_version = qcom_cpufreq_kryo_get_msm_id();
> + if (NUM_OF_MSM8996_VERSIONS == msm8996_version) {
> + dev_err(cpu_dev_silver, "Not Snapdragon 820/821!");
> + return -ENODEV;
> + }
> +
> + np = dev_pm_opp_of_get_opp_desc_node(cpu_dev_silver);
> + if (IS_ERR_OR_NULL(np))
> + return PTR_ERR(np);
> +
> + if (!of_device_is_compatible(np, "operating-points-v2-kryo-cpu")) {
> + ret = -ENOENT;
> + goto free_np;
> + }
> +
> + speedbin_nvmem = of_nvmem_cell_get(np, NULL);
> + if (IS_ERR(speedbin_nvmem)) {
> + ret = PTR_ERR(speedbin_nvmem);
> + dev_err(cpu_dev_silver, "Could not get nvmem cell: %d\n", ret);
> + goto free_np;
> + }
> +
> + speedbin = nvmem_cell_read(speedbin_nvmem, &len);
> + nvmem_cell_put(speedbin_nvmem);
> +
> + switch (msm8996_version) {
> + case MSM8996_V3:
> + versions = 1 << (unsigned int)(*speedbin);
> + break;
> + case MSM8996_SG:
> + versions = 1 << ((unsigned int)(*speedbin) + 4);
> + break;
> + default:
> + BUG();
> + break;
> + }
> +
> + opp_silver = dev_pm_opp_set_supported_hw(cpu_dev_silver,&versions,1);
> + if (IS_ERR_OR_NULL(opp_silver)) {
This API doesn't return NULL and so IS_ERR() would be sufficient.
> + dev_err(cpu_dev_silver, "Failed to set supported hardware\n");
> + ret = PTR_ERR(opp_silver);
> + goto free_np;
> + }
> +
> + opp_gold = dev_pm_opp_set_supported_hw(cpu_dev_gold,&versions,1);
> + if (IS_ERR_OR_NULL(opp_gold)) {
same here.
> + dev_err(cpu_dev_gold, "Failed to set supported hardware\n");
> + ret = PTR_ERR(opp_gold);
> + goto free_opp_silver;
> + }
> +
> + pdev = platform_device_register_simple("cpufreq-dt", -1, NULL, 0);
> + if (!IS_ERR_OR_NULL(pdev))
> + goto out;
Simply return from here and remove the useless label out.
> +
> + ret = PTR_ERR(pdev);
> + dev_err(cpu_dev_silver, "Failed to register platform device\n");
> + dev_pm_opp_put_supported_hw(opp_gold);
> +
> +free_opp_silver:
> + dev_pm_opp_put_supported_hw(opp_silver);
> +
> +free_np:
> + of_node_put(np);
> +
> +out:
> + return ret;
> +}
> +late_initcall(qcom_cpufreq_kryo_driver_init);
Please resend only this patch now or just paste the new code in a mail
here so that I can review it quickly and then you can resend the final
version. Most of the patches aren't changing anyway.
--
viresh
^ permalink raw reply
* Re: [PATCH v5 4/4] drm/rockchip: support dp training outside dp firmware
From: hl @ 2018-05-18 1:41 UTC (permalink / raw)
To: Sean Paul
Cc: devicetree, heiko, airlied, briannorris, eballetbo, dianders,
jani.nikula, linux-kernel, linux-rockchip, robh+dt, dri-devel,
zyw, daniel.vetter, linux-arm-kernel, Kishon Vijay Abraham I
In-Reply-To: <20180517135136.GD3373@art_vandelay>
+ Kishon
On Thursday, May 17, 2018 09:51 PM, Sean Paul wrote:
> On Thu, May 17, 2018 at 05:18:00PM +0800, Lin Huang wrote:
>> DP firmware uses fixed phy config values to do training, but some
>> boards need to adjust these values to fit for their unique hardware
>> design. So get phy config values from dts and use software link training
>> instead of relying on firmware, if software training fail, keep firmware
>> training as a fallback if sw training fails.
>>
>> Signed-off-by: Chris Zhong <zyw@rock-chips.com>
>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>> ---
>> Changes in v2:
>> - update patch following Enric suggest
>> Changes in v3:
>> - use variable fw_training instead sw_training_success
>> - base on DP SPCE, if training fail use lower link rate to retry training
>> Changes in v4:
>> - improve cdn_dp_get_lower_link_rate() and cdn_dp_software_train_link() follow Sean suggest
>> Changes in v5:
>> - fix some whitespcae issue
>>
>> drivers/gpu/drm/rockchip/Makefile | 3 +-
>> drivers/gpu/drm/rockchip/cdn-dp-core.c | 24 +-
>> drivers/gpu/drm/rockchip/cdn-dp-core.h | 2 +
>> drivers/gpu/drm/rockchip/cdn-dp-link-training.c | 420 ++++++++++++++++++++++++
>> drivers/gpu/drm/rockchip/cdn-dp-reg.c | 31 +-
>> drivers/gpu/drm/rockchip/cdn-dp-reg.h | 38 ++-
>> 6 files changed, 505 insertions(+), 13 deletions(-)
>> create mode 100644 drivers/gpu/drm/rockchip/cdn-dp-link-training.c
>>
>> diff --git a/drivers/gpu/drm/rockchip/Makefile b/drivers/gpu/drm/rockchip/Makefile
>> index a314e21..b932f62 100644
>> --- a/drivers/gpu/drm/rockchip/Makefile
>> +++ b/drivers/gpu/drm/rockchip/Makefile
>> @@ -9,7 +9,8 @@ rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o \
>> rockchipdrm-$(CONFIG_DRM_FBDEV_EMULATION) += rockchip_drm_fbdev.o
>>
>> rockchipdrm-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o
>> -rockchipdrm-$(CONFIG_ROCKCHIP_CDN_DP) += cdn-dp-core.o cdn-dp-reg.o
>> +rockchipdrm-$(CONFIG_ROCKCHIP_CDN_DP) += cdn-dp-core.o cdn-dp-reg.o \
>> + cdn-dp-link-training.o
>> rockchipdrm-$(CONFIG_ROCKCHIP_DW_HDMI) += dw_hdmi-rockchip.o
>> rockchipdrm-$(CONFIG_ROCKCHIP_DW_MIPI_DSI) += dw-mipi-dsi.o
>> rockchipdrm-$(CONFIG_ROCKCHIP_INNO_HDMI) += inno_hdmi.o
>> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c
>> index cce64c1..d9d0d4d 100644
>> --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
>> +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c
>> @@ -629,11 +629,13 @@ static void cdn_dp_encoder_enable(struct drm_encoder *encoder)
>> goto out;
>> }
>> }
>> -
>> - ret = cdn_dp_set_video_status(dp, CONTROL_VIDEO_IDLE);
>> - if (ret) {
>> - DRM_DEV_ERROR(dp->dev, "Failed to idle video %d\n", ret);
>> - goto out;
>> + if (dp->use_fw_training == true) {
>> + ret = cdn_dp_set_video_status(dp, CONTROL_VIDEO_IDLE);
>> + if (ret) {
>> + DRM_DEV_ERROR(dp->dev,
>> + "Failed to idle video %d\n", ret);
>> + goto out;
>> + }
>> }
>>
>> ret = cdn_dp_config_video(dp);
>> @@ -642,11 +644,15 @@ static void cdn_dp_encoder_enable(struct drm_encoder *encoder)
>> goto out;
>> }
>>
>> - ret = cdn_dp_set_video_status(dp, CONTROL_VIDEO_VALID);
>> - if (ret) {
>> - DRM_DEV_ERROR(dp->dev, "Failed to valid video %d\n", ret);
>> - goto out;
>> + if (dp->use_fw_training == true) {
>> + ret = cdn_dp_set_video_status(dp, CONTROL_VIDEO_VALID);
>> + if (ret) {
>> + DRM_DEV_ERROR(dp->dev,
>> + "Failed to valid video %d\n", ret);
>> + goto out;
>> + }
>> }
>> +
>> out:
>> mutex_unlock(&dp->lock);
>> }
>> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.h b/drivers/gpu/drm/rockchip/cdn-dp-core.h
>> index 46159b2..77a9793 100644
>> --- a/drivers/gpu/drm/rockchip/cdn-dp-core.h
>> +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.h
>> @@ -84,6 +84,7 @@ struct cdn_dp_device {
>> bool connected;
>> bool active;
>> bool suspended;
>> + bool use_fw_training;
>>
>> const struct firmware *fw; /* cdn dp firmware */
>> unsigned int fw_version; /* cdn fw version */
>> @@ -106,6 +107,7 @@ struct cdn_dp_device {
>> u8 ports;
>> u8 lanes;
>> int active_port;
>> + u8 train_set[4];
>>
>> u8 dpcd[DP_RECEIVER_CAP_SIZE];
>> bool sink_has_audio;
>> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-link-training.c b/drivers/gpu/drm/rockchip/cdn-dp-link-training.c
>> new file mode 100644
>> index 0000000..73c3290
>> --- /dev/null
>> +++ b/drivers/gpu/drm/rockchip/cdn-dp-link-training.c
>> @@ -0,0 +1,420 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
>> + * Author: Chris Zhong <zyw@rock-chips.com>
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/delay.h>
>> +#include <linux/phy/phy.h>
>> +#include <soc/rockchip/rockchip_phy_typec.h>
>> +
>> +#include "cdn-dp-core.h"
>> +#include "cdn-dp-reg.h"
>> +
>> +static void cdn_dp_set_signal_levels(struct cdn_dp_device *dp)
>> +{
>> + struct cdn_dp_port *port = dp->port[dp->active_port];
>> + struct rockchip_typec_phy *tcphy = phy_get_drvdata(port->phy);
> You ignored Brian's comment on the previous patch:
> This is still antithetical to the PHY framework; you're assuming that
> this is a particular type of PHY here.
>
> FWIW, the mediatek drm driver also assumes a certain PHY type. A quick grep of
> drivers/ shows that the only other non-phy/ driver using this function
> (pinctrl-tegra-xusb.c) also casts it.
>
> Sean
Thanks Sean, except phy framework have new API to handle it, i have not
idea how to do it in a better way.
>> +
>> + int rate = drm_dp_bw_code_to_link_rate(dp->link.rate);
>> + u8 swing = (dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK) >>
>> + DP_TRAIN_VOLTAGE_SWING_SHIFT;
>> + u8 pre_emphasis = (dp->train_set[0] & DP_TRAIN_PRE_EMPHASIS_MASK)
>> + >> DP_TRAIN_PRE_EMPHASIS_SHIFT;
>> +
>> + tcphy->typec_phy_config(port->phy, rate, dp->link.num_lanes,
>> + swing, pre_emphasis);
>> +}
>> +
>> +static int cdn_dp_set_pattern(struct cdn_dp_device *dp, uint8_t dp_train_pat)
>> +{
>> + u32 phy_config, global_config;
>> + int ret;
>> + uint8_t pattern = dp_train_pat & DP_TRAINING_PATTERN_MASK;
>> +
>> + global_config = NUM_LANES(dp->link.num_lanes - 1) | SST_MODE |
>> + GLOBAL_EN | RG_EN | ENC_RST_DIS | WR_VHSYNC_FALL;
>> +
>> + phy_config = DP_TX_PHY_ENCODER_BYPASS(0) |
>> + DP_TX_PHY_SKEW_BYPASS(0) |
>> + DP_TX_PHY_DISPARITY_RST(0) |
>> + DP_TX_PHY_LANE0_SKEW(0) |
>> + DP_TX_PHY_LANE1_SKEW(1) |
>> + DP_TX_PHY_LANE2_SKEW(2) |
>> + DP_TX_PHY_LANE3_SKEW(3) |
>> + DP_TX_PHY_10BIT_ENABLE(0);
>> +
>> + if (pattern != DP_TRAINING_PATTERN_DISABLE) {
>> + global_config |= NO_VIDEO;
>> + phy_config |= DP_TX_PHY_TRAINING_ENABLE(1) |
>> + DP_TX_PHY_SCRAMBLER_BYPASS(1) |
>> + DP_TX_PHY_TRAINING_PATTERN(pattern);
>> + }
>> +
>> + ret = cdn_dp_reg_write(dp, DP_FRAMER_GLOBAL_CONFIG, global_config);
>> + if (ret) {
>> + DRM_ERROR("fail to set DP_FRAMER_GLOBAL_CONFIG, error: %d\n",
>> + ret);
>> + return ret;
>> + }
>> +
>> + ret = cdn_dp_reg_write(dp, DP_TX_PHY_CONFIG_REG, phy_config);
>> + if (ret) {
>> + DRM_ERROR("fail to set DP_TX_PHY_CONFIG_REG, error: %d\n",
>> + ret);
>> + return ret;
>> + }
>> +
>> + ret = cdn_dp_reg_write(dp, DPTX_LANE_EN, BIT(dp->link.num_lanes) - 1);
>> + if (ret) {
>> + DRM_ERROR("fail to set DPTX_LANE_EN, error: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + if (drm_dp_enhanced_frame_cap(dp->dpcd))
>> + ret = cdn_dp_reg_write(dp, DPTX_ENHNCD, 1);
>> + else
>> + ret = cdn_dp_reg_write(dp, DPTX_ENHNCD, 0);
>> + if (ret)
>> + DRM_ERROR("failed to set DPTX_ENHNCD, error: %x\n", ret);
>> +
>> + return ret;
>> +}
>> +
>> +static u8 cdn_dp_pre_emphasis_max(u8 voltage_swing)
>> +{
>> + switch (voltage_swing & DP_TRAIN_VOLTAGE_SWING_MASK) {
>> + case DP_TRAIN_VOLTAGE_SWING_LEVEL_0:
>> + return DP_TRAIN_PRE_EMPH_LEVEL_3;
>> + case DP_TRAIN_VOLTAGE_SWING_LEVEL_1:
>> + return DP_TRAIN_PRE_EMPH_LEVEL_2;
>> + case DP_TRAIN_VOLTAGE_SWING_LEVEL_2:
>> + return DP_TRAIN_PRE_EMPH_LEVEL_1;
>> + default:
>> + return DP_TRAIN_PRE_EMPH_LEVEL_0;
>> + }
>> +}
>> +
>> +static void cdn_dp_get_adjust_train(struct cdn_dp_device *dp,
>> + uint8_t link_status[DP_LINK_STATUS_SIZE])
>> +{
>> + int i;
>> + uint8_t v = 0, p = 0;
>> + uint8_t preemph_max;
>> +
>> + for (i = 0; i < dp->link.num_lanes; i++) {
>> + v = max(v, drm_dp_get_adjust_request_voltage(link_status, i));
>> + p = max(p, drm_dp_get_adjust_request_pre_emphasis(link_status,
>> + i));
>> + }
>> +
>> + if (v >= VOLTAGE_LEVEL_2)
>> + v = VOLTAGE_LEVEL_2 | DP_TRAIN_MAX_SWING_REACHED;
>> +
>> + preemph_max = cdn_dp_pre_emphasis_max(v);
>> + if (p >= preemph_max)
>> + p = preemph_max | DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
>> +
>> + for (i = 0; i < dp->link.num_lanes; i++)
>> + dp->train_set[i] = v | p;
>> +}
>> +
>> +/*
>> + * Pick training pattern for channel equalization. Training Pattern 3 for HBR2
>> + * or 1.2 devices that support it, Training Pattern 2 otherwise.
>> + */
>> +static u32 cdn_dp_select_chaneq_pattern(struct cdn_dp_device *dp)
>> +{
>> + u32 training_pattern = DP_TRAINING_PATTERN_2;
>> +
>> + /*
>> + * cdn dp support HBR2 also support TPS3. TPS3 support is also mandatory
>> + * for downstream devices that support HBR2. However, not all sinks
>> + * follow the spec.
>> + */
>> + if (drm_dp_tps3_supported(dp->dpcd))
>> + training_pattern = DP_TRAINING_PATTERN_3;
>> + else
>> + DRM_DEBUG_KMS("5.4 Gbps link rate without sink TPS3 support\n");
>> +
>> + return training_pattern;
>> +}
>> +
>> +
>> +static bool cdn_dp_link_max_vswing_reached(struct cdn_dp_device *dp)
>> +{
>> + int lane;
>> +
>> + for (lane = 0; lane < dp->link.num_lanes; lane++)
>> + if ((dp->train_set[lane] & DP_TRAIN_MAX_SWING_REACHED) == 0)
>> + return false;
>> +
>> + return true;
>> +}
>> +
>> +static int cdn_dp_update_link_train(struct cdn_dp_device *dp)
>> +{
>> + int ret;
>> +
>> + cdn_dp_set_signal_levels(dp);
>> +
>> + ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_LANE0_SET,
>> + dp->train_set, dp->link.num_lanes);
>> + if (ret != dp->link.num_lanes)
>> + return -EINVAL;
>> +
>> + return 0;
>> +}
>> +
>> +static int cdn_dp_set_link_train(struct cdn_dp_device *dp,
>> + uint8_t dp_train_pat)
>> +{
>> + uint8_t buf[sizeof(dp->train_set) + 1];
>> + int ret, len;
>> +
>> + buf[0] = dp_train_pat;
>> + if ((dp_train_pat & DP_TRAINING_PATTERN_MASK) ==
>> + DP_TRAINING_PATTERN_DISABLE) {
>> + /* don't write DP_TRAINING_LANEx_SET on disable */
>> + len = 1;
>> + } else {
>> + /* DP_TRAINING_LANEx_SET follow DP_TRAINING_PATTERN_SET */
>> + memcpy(buf + 1, dp->train_set, dp->link.num_lanes);
>> + len = dp->link.num_lanes + 1;
>> + }
>> +
>> + ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_PATTERN_SET,
>> + buf, len);
>> + if (ret != len)
>> + return -EINVAL;
>> +
>> + return 0;
>> +}
>> +
>> +static int cdn_dp_reset_link_train(struct cdn_dp_device *dp,
>> + uint8_t dp_train_pat)
>> +{
>> + int ret;
>> +
>> + memset(dp->train_set, 0, sizeof(dp->train_set));
>> +
>> + cdn_dp_set_signal_levels(dp);
>> +
>> + ret = cdn_dp_set_pattern(dp, dp_train_pat);
>> + if (ret)
>> + return ret;
>> +
>> + return cdn_dp_set_link_train(dp, dp_train_pat);
>> +}
>> +
>> +/* Enable corresponding port and start training pattern 1 */
>> +static int cdn_dp_link_training_clock_recovery(struct cdn_dp_device *dp)
>> +{
>> + u8 voltage;
>> + u8 link_status[DP_LINK_STATUS_SIZE];
>> + u32 voltage_tries, max_vswing_tries;
>> + int ret;
>> +
>> + /* clock recovery */
>> + ret = cdn_dp_reset_link_train(dp, DP_TRAINING_PATTERN_1 |
>> + DP_LINK_SCRAMBLING_DISABLE);
>> + if (ret) {
>> + DRM_ERROR("failed to start link train\n");
>> + return ret;
>> + }
>> +
>> + voltage_tries = 1;
>> + max_vswing_tries = 0;
>> + for (;;) {
>> + drm_dp_link_train_clock_recovery_delay(dp->dpcd);
>> + if (drm_dp_dpcd_read_link_status(&dp->aux, link_status) !=
>> + DP_LINK_STATUS_SIZE) {
>> + DRM_ERROR("failed to get link status\n");
>> + return -EINVAL;
>> + }
>> +
>> + if (drm_dp_clock_recovery_ok(link_status, dp->link.num_lanes)) {
>> + DRM_DEBUG_KMS("clock recovery OK\n");
>> + return 0;
>> + }
>> +
>> + if (voltage_tries >= 5) {
>> + DRM_DEBUG_KMS("Same voltage tried 5 times\n");
>> + return -EINVAL;
>> + }
>> +
>> + if (max_vswing_tries >= 1) {
>> + DRM_DEBUG_KMS("Max Voltage Swing reached\n");
>> + return -EINVAL;
>> + }
>> +
>> + voltage = dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK;
>> +
>> + /* Update training set as requested by target */
>> + cdn_dp_get_adjust_train(dp, link_status);
>> + if (cdn_dp_update_link_train(dp)) {
>> + DRM_ERROR("failed to update link training\n");
>> + return -EINVAL;
>> + }
>> +
>> + if ((dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK) ==
>> + voltage)
>> + ++voltage_tries;
>> + else
>> + voltage_tries = 1;
>> +
>> + if (cdn_dp_link_max_vswing_reached(dp))
>> + ++max_vswing_tries;
>> + }
>> +}
>> +
>> +static int cdn_dp_link_training_channel_equalization(struct cdn_dp_device *dp)
>> +{
>> + int tries, ret;
>> + u32 training_pattern;
>> + uint8_t link_status[DP_LINK_STATUS_SIZE];
>> +
>> + training_pattern = cdn_dp_select_chaneq_pattern(dp);
>> + training_pattern |= DP_LINK_SCRAMBLING_DISABLE;
>> +
>> + ret = cdn_dp_set_pattern(dp, training_pattern);
>> + if (ret)
>> + return ret;
>> +
>> + ret = cdn_dp_set_link_train(dp, training_pattern);
>> + if (ret) {
>> + DRM_ERROR("failed to start channel equalization\n");
>> + return ret;
>> + }
>> +
>> + for (tries = 0; tries < 5; tries++) {
>> + drm_dp_link_train_channel_eq_delay(dp->dpcd);
>> + if (drm_dp_dpcd_read_link_status(&dp->aux, link_status) !=
>> + DP_LINK_STATUS_SIZE) {
>> + DRM_ERROR("failed to get link status\n");
>> + break;
>> + }
>> +
>> + /* Make sure clock is still ok */
>> + if (!drm_dp_clock_recovery_ok(link_status,
>> + dp->link.num_lanes)) {
>> + DRM_DEBUG_KMS("Clock recovery check failed\n");
>> + break;
>> + }
>> +
>> + if (drm_dp_channel_eq_ok(link_status, dp->link.num_lanes)) {
>> + DRM_DEBUG_KMS("Channel EQ done\n");
>> + return 0;
>> + }
>> +
>> + /* Update training set as requested by target */
>> + cdn_dp_get_adjust_train(dp, link_status);
>> + if (cdn_dp_update_link_train(dp)) {
>> + DRM_ERROR("failed to update link training\n");
>> + break;
>> + }
>> + }
>> +
>> + /* Try 5 times, else fail and try at lower BW */
>> + if (tries == 5)
>> + DRM_DEBUG_KMS("Channel equalization failed 5 times\n");
>> +
>> + return -EINVAL;
>> +}
>> +
>> +static int cdn_dp_stop_link_train(struct cdn_dp_device *dp)
>> +{
>> + int ret = cdn_dp_set_pattern(dp, DP_TRAINING_PATTERN_DISABLE);
>> +
>> + if (ret)
>> + return ret;
>> +
>> + return cdn_dp_set_link_train(dp, DP_TRAINING_PATTERN_DISABLE);
>> +}
>> +
>> +static int cdn_dp_get_lower_link_rate(struct cdn_dp_device *dp)
>> +{
>> + switch (dp->link.rate) {
>> + case DP_LINK_BW_1_62:
>> + return -EINVAL;
>> + case DP_LINK_BW_2_7:
>> + dp->link.rate = DP_LINK_BW_1_62;
>> + break;
>> + case DP_LINK_BW_5_4:
>> + dp->link.rate = DP_LINK_BW_2_7;
>> + break;
>> + default:
>> + dp->link.rate = DP_LINK_BW_5_4;
>> + break;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +int cdn_dp_software_train_link(struct cdn_dp_device *dp)
>> +{
>> + int ret, stop_err;
>> + u8 link_config[2];
>> + u32 rate, sink_max, source_max;
>> +
>> + ret = drm_dp_dpcd_read(&dp->aux, DP_DPCD_REV, dp->dpcd,
>> + sizeof(dp->dpcd));
>> + if (ret < 0) {
>> + DRM_DEV_ERROR(dp->dev, "Failed to get caps %d\n", ret);
>> + return ret;
>> + }
>> +
>> + source_max = dp->lanes;
>> + sink_max = drm_dp_max_lane_count(dp->dpcd);
>> + dp->link.num_lanes = min(source_max, sink_max);
>> +
>> + source_max = drm_dp_bw_code_to_link_rate(CDN_DP_MAX_LINK_RATE);
>> + sink_max = drm_dp_max_link_rate(dp->dpcd);
>> + rate = min(source_max, sink_max);
>> + dp->link.rate = drm_dp_link_rate_to_bw_code(rate);
>> +
>> + link_config[0] = 0;
>> + link_config[1] = 0;
>> + if (dp->dpcd[DP_MAIN_LINK_CHANNEL_CODING] & 0x01)
>> + link_config[1] = DP_SET_ANSI_8B10B;
>> + drm_dp_dpcd_write(&dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2);
>> +
>> + while (true) {
>> +
>> + /* Write the link configuration data */
>> + link_config[0] = dp->link.rate;
>> + link_config[1] = dp->link.num_lanes;
>> + if (drm_dp_enhanced_frame_cap(dp->dpcd))
>> + link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
>> + drm_dp_dpcd_write(&dp->aux, DP_LINK_BW_SET, link_config, 2);
>> +
>> + ret = cdn_dp_link_training_clock_recovery(dp);
>> + if (ret) {
>> + if (!cdn_dp_get_lower_link_rate(dp))
>> + continue;
>> +
>> + DRM_ERROR("training clock recovery failed: %d\n", ret);
>> + break;
>> + }
>> +
>> + ret = cdn_dp_link_training_channel_equalization(dp);
>> + if (ret) {
>> + if (!cdn_dp_get_lower_link_rate(dp))
>> + continue;
>> +
>> + DRM_ERROR("training channel eq failed: %d\n", ret);
>> + break;
>> + }
>> +
>> + break;
>> + }
>> +
>> + stop_err = cdn_dp_stop_link_train(dp);
>> + if (stop_err) {
>> + DRM_ERROR("stop training fail, error: %d\n", stop_err);
>> + return stop_err;
>> + }
>> +
>> + return ret;
>> +}
>> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-reg.c b/drivers/gpu/drm/rockchip/cdn-dp-reg.c
>> index 979355d..e1273e6 100644
>> --- a/drivers/gpu/drm/rockchip/cdn-dp-reg.c
>> +++ b/drivers/gpu/drm/rockchip/cdn-dp-reg.c
>> @@ -17,7 +17,9 @@
>> #include <linux/delay.h>
>> #include <linux/io.h>
>> #include <linux/iopoll.h>
>> +#include <linux/phy/phy.h>
>> #include <linux/reset.h>
>> +#include <soc/rockchip/rockchip_phy_typec.h>
>>
>> #include "cdn-dp-core.h"
>> #include "cdn-dp-reg.h"
>> @@ -189,7 +191,7 @@ static int cdn_dp_mailbox_send(struct cdn_dp_device *dp, u8 module_id,
>> return 0;
>> }
>>
>> -static int cdn_dp_reg_write(struct cdn_dp_device *dp, u16 addr, u32 val)
>> +int cdn_dp_reg_write(struct cdn_dp_device *dp, u16 addr, u32 val)
>> {
>> u8 msg[6];
>>
>> @@ -609,6 +611,31 @@ int cdn_dp_train_link(struct cdn_dp_device *dp)
>> {
>> int ret;
>>
>> + /*
>> + * DP firmware uses fixed phy config values to do training, but some
>> + * boards need to adjust these values to fit for their unique hardware
>> + * design. So if the phy is using custom config values, do software
>> + * link training instead of relying on firmware, if software training
>> + * fail, keep firmware training as a fallback if sw training fails.
>> + */
>> + ret = cdn_dp_software_train_link(dp);
>> + if (ret) {
>> + DRM_DEV_ERROR(dp->dev,
>> + "Failed to do software training %d\n", ret);
>> + goto do_fw_training;
>> + }
>> + ret = cdn_dp_reg_write(dp, SOURCE_HDTX_CAR, 0xf);
>> + if (ret) {
>> + DRM_DEV_ERROR(dp->dev,
>> + "Failed to write SOURCE_HDTX_CAR register %d\n", ret);
>> + goto do_fw_training;
>> + }
>> + dp->use_fw_training = false;
>> + return 0;
>> +
>> +do_fw_training:
>> + dp->use_fw_training = true;
>> + DRM_DEV_DEBUG_KMS(dp->dev, "use fw training\n");
>> ret = cdn_dp_training_start(dp);
>> if (ret) {
>> DRM_DEV_ERROR(dp->dev, "Failed to start training %d\n", ret);
>> @@ -623,7 +650,7 @@ int cdn_dp_train_link(struct cdn_dp_device *dp)
>>
>> DRM_DEV_DEBUG_KMS(dp->dev, "rate:0x%x, lanes:%d\n", dp->link.rate,
>> dp->link.num_lanes);
>> - return ret;
>> + return 0;
>> }
>>
>> int cdn_dp_set_video_status(struct cdn_dp_device *dp, int active)
>> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-reg.h b/drivers/gpu/drm/rockchip/cdn-dp-reg.h
>> index 6580b11..3420771 100644
>> --- a/drivers/gpu/drm/rockchip/cdn-dp-reg.h
>> +++ b/drivers/gpu/drm/rockchip/cdn-dp-reg.h
>> @@ -137,7 +137,7 @@
>> #define HPD_EVENT_MASK 0x211c
>> #define HPD_EVENT_DET 0x2120
>>
>> -/* dpyx framer addr */
>> +/* dptx framer addr */
>> #define DP_FRAMER_GLOBAL_CONFIG 0x2200
>> #define DP_SW_RESET 0x2204
>> #define DP_FRAMER_TU 0x2208
>> @@ -431,6 +431,40 @@
>> /* Reference cycles when using lane clock as reference */
>> #define LANE_REF_CYC 0x8000
>>
>> +/* register CM_VID_CTRL */
>> +#define LANE_VID_REF_CYC(x) (((x) & (BIT(24) - 1)) << 0)
>> +#define NMVID_MEAS_TOLERANCE(x) (((x) & 0xf) << 24)
>> +
>> +/* register DP_TX_PHY_CONFIG_REG */
>> +#define DP_TX_PHY_TRAINING_ENABLE(x) ((x) & 1)
>> +#define DP_TX_PHY_TRAINING_TYPE_PRBS7 (0 << 1)
>> +#define DP_TX_PHY_TRAINING_TYPE_TPS1 (1 << 1)
>> +#define DP_TX_PHY_TRAINING_TYPE_TPS2 (2 << 1)
>> +#define DP_TX_PHY_TRAINING_TYPE_TPS3 (3 << 1)
>> +#define DP_TX_PHY_TRAINING_TYPE_TPS4 (4 << 1)
>> +#define DP_TX_PHY_TRAINING_TYPE_PLTPAT (5 << 1)
>> +#define DP_TX_PHY_TRAINING_TYPE_D10_2 (6 << 1)
>> +#define DP_TX_PHY_TRAINING_TYPE_HBR2CPAT (8 << 1)
>> +#define DP_TX_PHY_TRAINING_PATTERN(x) ((x) << 1)
>> +#define DP_TX_PHY_SCRAMBLER_BYPASS(x) (((x) & 1) << 5)
>> +#define DP_TX_PHY_ENCODER_BYPASS(x) (((x) & 1) << 6)
>> +#define DP_TX_PHY_SKEW_BYPASS(x) (((x) & 1) << 7)
>> +#define DP_TX_PHY_DISPARITY_RST(x) (((x) & 1) << 8)
>> +#define DP_TX_PHY_LANE0_SKEW(x) (((x) & 7) << 9)
>> +#define DP_TX_PHY_LANE1_SKEW(x) (((x) & 7) << 12)
>> +#define DP_TX_PHY_LANE2_SKEW(x) (((x) & 7) << 15)
>> +#define DP_TX_PHY_LANE3_SKEW(x) (((x) & 7) << 18)
>> +#define DP_TX_PHY_10BIT_ENABLE(x) (((x) & 1) << 21)
>> +
>> +/* register DP_FRAMER_GLOBAL_CONFIG */
>> +#define NUM_LANES(x) ((x) & 3)
>> +#define SST_MODE (0 << 2)
>> +#define RG_EN (0 << 4)
>> +#define GLOBAL_EN BIT(3)
>> +#define NO_VIDEO BIT(5)
>> +#define ENC_RST_DIS BIT(6)
>> +#define WR_VHSYNC_FALL BIT(7)
>> +
>> enum voltage_swing_level {
>> VOLTAGE_LEVEL_0,
>> VOLTAGE_LEVEL_1,
>> @@ -476,6 +510,7 @@ int cdn_dp_set_host_cap(struct cdn_dp_device *dp, u8 lanes, bool flip);
>> int cdn_dp_event_config(struct cdn_dp_device *dp);
>> u32 cdn_dp_get_event(struct cdn_dp_device *dp);
>> int cdn_dp_get_hpd_status(struct cdn_dp_device *dp);
>> +int cdn_dp_reg_write(struct cdn_dp_device *dp, u16 addr, u32 val);
>> ssize_t cdn_dp_dpcd_write(struct cdn_dp_device *dp, u32 addr,
>> u8 *data, u16 len);
>> ssize_t cdn_dp_dpcd_read(struct cdn_dp_device *dp, u32 addr,
>> @@ -489,4 +524,5 @@ int cdn_dp_config_video(struct cdn_dp_device *dp);
>> int cdn_dp_audio_stop(struct cdn_dp_device *dp, struct audio_info *audio);
>> int cdn_dp_audio_mute(struct cdn_dp_device *dp, bool enable);
>> int cdn_dp_audio_config(struct cdn_dp_device *dp, struct audio_info *audio);
>> +int cdn_dp_software_train_link(struct cdn_dp_device *dp);
>> #endif /* _CDN_DP_REG_H */
>> --
>> 2.7.4
>>
^ permalink raw reply
* RE: [PATCH 1/2] clk: imx7d: correct enet clock CCGR register offset
From: Anson Huang @ 2018-05-18 1:06 UTC (permalink / raw)
To: Stefan Agner
Cc: shawnguo@kernel.org, kernel@pengutronix.de, Fabio Estevam,
robh+dt@kernel.org, mark.rutland@arm.com, mturquette@baylibre.com,
sboyd@kernel.org, Adriana Reus, rui.silva@linaro.org,
dl-linux-imx, linux-arm-kernel@lists.infradead.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-clk@vger.kernel.org
In-Reply-To: <96f2ab49e1a9c03ccd477346b8285c76@agner.ch>
Hi, Stefan
Anson Huang
Best Regards!
> -----Original Message-----
> From: Stefan Agner [mailto:stefan@agner.ch]
> Sent: Thursday, May 17, 2018 7:22 PM
> To: Anson Huang <anson.huang@nxp.com>
> Cc: shawnguo@kernel.org; kernel@pengutronix.de; Fabio Estevam
> <fabio.estevam@nxp.com>; robh+dt@kernel.org; mark.rutland@arm.com;
> mturquette@baylibre.com; sboyd@kernel.org; Adriana Reus
> <adriana.reus@nxp.com>; rui.silva@linaro.org; dl-linux-imx
> <linux-imx@nxp.com>; linux-arm-kernel@lists.infradead.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-clk@vger.kernel.org
> Subject: Re: [PATCH 1/2] clk: imx7d: correct enet clock CCGR register offset
>
> On 17.05.2018 10:40, Anson Huang wrote:
> > Correct enet clock CCGR register offset.
> >
> > CCGR6: IMX7D_ENET_AXI_ROOT_CLK (enet1 and enet2 bus clocks)
> > CCGR112: IMX7D_ENET1_TIME_ROOT_CLK, IMX7D_ENET1_IPG_ROOT_CLK
> > CCGR113: IMX7D_ENET2_TIME_ROOT_CLK, IMX7D_ENET2_IPG_ROOT_CLK
> >
> > IMX7D_ENET_PHY_REF_ROOT_DIV supplies clock for PHY, no gate after this
> > clock, its parent clock root has gate.
> > IMX7D_ENET1_REF_ROOT_DIV/IMX7D_ENET2_REF_ROOT_DIV supplies
> clocks for
> > enet IPG_CLK_RMII, no gate after the clock, its parent clock root has
> > gate.
> >
> > IMX7D_PLL_ENET_MAIN_125M_CLK (anatop pll) supplies clock for enet
> > RGMII tx_clk.
>
> As far as I can tell there are two changes here in one patch:
>
> 1. The non existing IMX7D_ENET_PHY_REF_ROOT_CLK gate is removed
>
> 2. Shared clock gate for the enet time/ipg clock is taken into account.
>
>
> I would rather prefer to have separate patches. The device tree change also
> does two things, so this would lead to 4 patches total.
>
> We can avoid the device tree change for the PHY clk and even maintain
> backward compatibility for that part by renaming
> IMX7D_ENET_PHY_REF_ROOT_DIV to IMX7D_ENET_PHY_REF_ROOT_CLK.
>
>
> So this would end up with the following first patch to address the PHY_ROOT
> clock issue:
>
> --- a/drivers/clk/imx/clk-imx7d.c
> +++ b/drivers/clk/imx/clk-imx7d.c
> @@ -738,7 +738,7 @@ static void __init imx7d_clocks_init(struct device_node
> *ccm_node)
> clks[IMX7D_ENET1_TIME_ROOT_DIV] =
> imx_clk_divider2("enet1_time_post_div", "enet1_time_pre_div", base +
> 0xa780, 0, 6);
> clks[IMX7D_ENET2_REF_ROOT_DIV] =
> imx_clk_divider2("enet2_ref_post_div", "enet2_ref_pre_div", base + 0xa800, 0,
> 6);
> clks[IMX7D_ENET2_TIME_ROOT_DIV] =
> imx_clk_divider2("enet2_time_post_div", "enet2_time_pre_div", base +
> 0xa880, 0, 6);
> - clks[IMX7D_ENET_PHY_REF_ROOT_DIV] =
> imx_clk_divider2("enet_phy_ref_post_div", "enet_phy_ref_pre_div", base +
> 0xa900, 0, 6);
> + clks[IMX7D_ENET_PHY_REF_ROOT_CLK] =
> imx_clk_divider2("enet_phy_ref_root_clk", "enet_phy_ref_pre_div", base +
> 0xa900, 0, 6);
> clks[IMX7D_EIM_ROOT_DIV] = imx_clk_divider2("eim_post_div",
> "eim_pre_div", base + 0xa980, 0, 6);
> clks[IMX7D_NAND_ROOT_CLK] = imx_clk_divider2("nand_root_clk",
> "nand_pre_div", base + 0xaa00, 0, 6);
> clks[IMX7D_QSPI_ROOT_DIV] = imx_clk_divider2("qspi_post_div",
> "qspi_pre_div", base + 0xaa80, 0, 6); @@ -816,7 +816,6 @@ static void __init
> imx7d_clocks_init(struct device_node *ccm_node)
> clks[IMX7D_ENET1_TIME_ROOT_CLK] =
> imx_clk_gate4("enet1_time_root_clk", "enet1_time_post_div", base + 0x44f0,
> 0);
> clks[IMX7D_ENET2_REF_ROOT_CLK] =
> imx_clk_gate4("enet2_ref_root_clk", "enet2_ref_post_div", base + 0x4500, 0);
> clks[IMX7D_ENET2_TIME_ROOT_CLK] =
> imx_clk_gate4("enet2_time_root_clk", "enet2_time_post_div", base + 0x4510,
> 0);
> - clks[IMX7D_ENET_PHY_REF_ROOT_CLK] =
> imx_clk_gate4("enet_phy_ref_root_clk", "enet_phy_ref_post_div", base +
> 0x4520, 0);
> clks[IMX7D_EIM_ROOT_CLK] = imx_clk_gate4("eim_root_clk",
> "eim_post_div", base + 0x4160, 0);
> clks[IMX7D_NAND_RAWNAND_CLK] =
> imx_clk_gate2_shared2("nand_rawnand_clk", "nand_root_clk", base + 0x4140,
> 0, &share_count_nand);
> clks[IMX7D_NAND_USDHC_BUS_RAWNAND_CLK] =
> imx_clk_gate2_shared2("nand_usdhc_rawnand_clk", "nand_usdhc_root_clk",
> base + 0x4140, 0, &share_count_nand);
>
>
> A second patch would then fix the clock gate issue and the third the
> unavoidable device tree change for the ipg clock.
>
> --
> Stefan
I follow your suggestion and re-structure the patch set to 3 patches, please help review it, thanks!
Anson.
>
>
> >
> > Based on Andy Duan's patch from the NXP kernel tree.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> > drivers/clk/imx/clk-imx7d.c | 11 ++++++-----
> > include/dt-bindings/clock/imx7d-clock.h | 4 +++-
> > 2 files changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/clk/imx/clk-imx7d.c b/drivers/clk/imx/clk-imx7d.c
> > index 975a20d..485ab49 100644
> > --- a/drivers/clk/imx/clk-imx7d.c
> > +++ b/drivers/clk/imx/clk-imx7d.c
> > @@ -26,6 +26,8 @@ static u32 share_count_sai1; static u32
> > share_count_sai2; static u32 share_count_sai3; static u32
> > share_count_nand;
> > +static u32 share_count_enet1;
> > +static u32 share_count_enet2;
> >
> > static const struct clk_div_table test_div_table[] = {
> > { .val = 3, .div = 1, },
> > @@ -805,6 +807,10 @@ static void __init imx7d_clocks_init(struct
> > device_node *ccm_node)
> > clks[IMX7D_MIPI_DSI_ROOT_CLK] = imx_clk_gate4("mipi_dsi_root_clk",
> > "mipi_dsi_post_div", base + 0x4650, 0);
> > clks[IMX7D_MIPI_CSI_ROOT_CLK] = imx_clk_gate4("mipi_csi_root_clk",
> > "mipi_csi_post_div", base + 0x4640, 0);
> > clks[IMX7D_MIPI_DPHY_ROOT_CLK] =
> imx_clk_gate4("mipi_dphy_root_clk",
> > "mipi_dphy_post_div", base + 0x4660, 0);
> > + clks[IMX7D_ENET1_IPG_ROOT_CLK] =
> > imx_clk_gate2_shared2("enet1_ipg_root_clk", "enet_axi_post_div", base
> > + 0x4700, 0, &share_count_enet1);
> > + clks[IMX7D_ENET1_TIME_ROOT_CLK] =
> > imx_clk_gate2_shared2("enet1_time_root_clk", "enet1_time_post_div",
> > base + 0x4700, 0, &share_count_enet1);
> > + clks[IMX7D_ENET2_IPG_ROOT_CLK] =
> > imx_clk_gate2_shared2("enet2_ipg_root_clk", "enet_axi_post_div", base
> > + 0x4710, 0, &share_count_enet2);
> > + clks[IMX7D_ENET2_TIME_ROOT_CLK] =
> > imx_clk_gate2_shared2("enet2_time_root_clk", "enet2_time_post_div",
> > base + 0x4710, 0, &share_count_enet2);
> > clks[IMX7D_SAI1_ROOT_CLK] = imx_clk_gate2_shared2("sai1_root_clk",
> > "sai1_post_div", base + 0x48c0, 0, &share_count_sai1);
> > clks[IMX7D_SAI1_IPG_CLK] = imx_clk_gate2_shared2("sai1_ipg_clk",
> > "ipg_root_clk", base + 0x48c0, 0, &share_count_sai1);
> > clks[IMX7D_SAI2_ROOT_CLK] = imx_clk_gate2_shared2("sai2_root_clk",
> > "sai2_post_div", base + 0x48d0, 0, &share_count_sai2); @@ -812,11
> > +818,6 @@ static void __init imx7d_clocks_init(struct device_node
> > *ccm_node)
> > clks[IMX7D_SAI3_ROOT_CLK] = imx_clk_gate2_shared2("sai3_root_clk",
> > "sai3_post_div", base + 0x48e0, 0, &share_count_sai3);
> > clks[IMX7D_SAI3_IPG_CLK] = imx_clk_gate2_shared2("sai3_ipg_clk",
> > "ipg_root_clk", base + 0x48e0, 0, &share_count_sai3);
> > clks[IMX7D_SPDIF_ROOT_CLK] = imx_clk_gate4("spdif_root_clk",
> > "spdif_post_div", base + 0x44d0, 0);
> > - clks[IMX7D_ENET1_REF_ROOT_CLK] =
> imx_clk_gate4("enet1_ref_root_clk",
> > "enet1_ref_post_div", base + 0x44e0, 0);
> > - clks[IMX7D_ENET1_TIME_ROOT_CLK] =
> > imx_clk_gate4("enet1_time_root_clk", "enet1_time_post_div", base +
> > 0x44f0, 0);
> > - clks[IMX7D_ENET2_REF_ROOT_CLK] =
> imx_clk_gate4("enet2_ref_root_clk",
> > "enet2_ref_post_div", base + 0x4500, 0);
> > - clks[IMX7D_ENET2_TIME_ROOT_CLK] =
> > imx_clk_gate4("enet2_time_root_clk", "enet2_time_post_div", base +
> > 0x4510, 0);
> > - clks[IMX7D_ENET_PHY_REF_ROOT_CLK] =
> > imx_clk_gate4("enet_phy_ref_root_clk", "enet_phy_ref_post_div", base +
> > 0x4520, 0);
> > clks[IMX7D_EIM_ROOT_CLK] = imx_clk_gate4("eim_root_clk",
> > "eim_post_div", base + 0x4160, 0);
> > clks[IMX7D_NAND_RAWNAND_CLK] =
> > imx_clk_gate2_shared2("nand_rawnand_clk", "nand_root_clk", base +
> > 0x4140, 0, &share_count_nand);
> > clks[IMX7D_NAND_USDHC_BUS_RAWNAND_CLK] =
> > imx_clk_gate2_shared2("nand_usdhc_rawnand_clk",
> "nand_usdhc_root_clk",
> > base + 0x4140, 0, &share_count_nand); diff --git
> > a/include/dt-bindings/clock/imx7d-clock.h
> > b/include/dt-bindings/clock/imx7d-clock.h
> > index b2325d3e2..fef0647 100644
> > --- a/include/dt-bindings/clock/imx7d-clock.h
> > +++ b/include/dt-bindings/clock/imx7d-clock.h
> > @@ -455,5 +455,7 @@
> > #define IMX7D_SNVS_CLK 442
> > #define IMX7D_CAAM_CLK 443
> > #define IMX7D_KPP_ROOT_CLK 444
> > -#define IMX7D_CLK_END 445
> > +#define IMX7D_ENET1_IPG_ROOT_CLK 445
> > +#define IMX7D_ENET2_IPG_ROOT_CLK 446
> > +#define IMX7D_CLK_END 447
> > #endif /* __DT_BINDINGS_CLOCK_IMX7D_H */
^ permalink raw reply
* Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings
From: Doug Anderson @ 2018-05-18 1:01 UTC (permalink / raw)
To: David Collins
Cc: Mark Brown, Liam Girdwood, Rob Herring, Mark Rutland,
linux-arm-msm, Linux ARM, devicetree, LKML, Rajendra Nayak,
Stephen Boyd
In-Reply-To: <869aad59-1cc5-28ef-1fb5-4ef846696c40@codeaurora.org>
Hi,
On Thu, May 17, 2018 at 5:16 PM, David Collins <collinsd@codeaurora.org> wrote:
> On 05/17/2018 02:22 PM, Doug Anderson wrote:
>> On Fri, May 11, 2018 at 7:28 PM, David Collins <collinsd@codeaurora.org> wrote:
>>> +- qcom,regulator-initial-microvolt
>>> + Usage: optional; VRM regulators only
>>> + Value type: <u32>
>>> + Definition: Specifies the initial voltage in microvolts to request for a
>>> + VRM regulator.
>>
>> Now that Mark has landed the patch adding support for the
>> -ENOTRECOVERABLE error code from get_voltage() / get_voltage_sel(), do
>> we still need the qcom,regulator-initial-microvolt property?
>
> Yes, this is still needed. The -ENOTRECOVERABLE patch ensures that
> qcom-rpmh-regulator devices can be registered even if
> qcom,regulator-initial-microvolt is not specified. However, that will
> result in the regulators being configured for the minimum voltage
> supported in the DT specified min/max range. The
> qcom,regulator-initial-microvolt property allows us to set a specific
> voltage that is larger than the min constraint.
Ah, OK. In the device tree fragment I saw the initial was always
equal to the min, so I wasn't sure if this was really needed in
practice. I presume it would only be important if a voltage was left
high by the bootloader for some peripheral that needs to continue to
function (and use the existing higher voltage) until a real device
claims it. For all other voltages, it should be fine if it's set to
the min until a real device claims it. Do you have real examples of
devices like this in boards using sdm845?
>> If this is really still needed, can it be moved to the regulator core?
>
> I'm not opposed to the idea, but I think that Mark is [1]:
Oh right. The downside of weeks between spins I guess. If Mark is
fine with the private property I won't fight it.
>>> +- regulator-initial-mode
>>> + Usage: optional; VRM regulators only
>>> + Value type: <u32>
>>> + Definition: Specifies the initial mode to request for a VRM regulator.
>>> + Supported values are RPMH_REGULATOR_MODE_* which are defined
>>> + in [1] (i.e. 0 to 3). This property may be specified even
>>> + if the regulator-allow-set-load property is not specified.
>>
>> Every time I read the above I wonder why you're documenting a standard
>> regulator regulator property in your bindings. ...then I realize it's
>> because you're doing it because you want to explicitly document what
>> the valid modes are. I wonder if it makes sense to just put a
>> reference somewhere else in this document to go look at the header
>> file where these are all nicely documented.
>
> Isn't that what the [1] in the above snippet is currently doing. Further
> down in qcom,rpmh-regulator.txt is this line:
>
> +[1] include/dt-bindings/regulator/qcom,rpmh-regulator.h
Right, but I want to move it so it doesn't look like you're defining a
property that's already defined in the common bindings. AKA get rid
of the "regulator-initial-mode" property description. Then add above
Examples:
========================
Regulator Modes
========================
RPMh regulators are designed to work with the standard regulator mode
bindings, using properties like "regulator-initial-mode". See
include/dt-bindings/regulator/qcom,rpmh-regulator.h for information on
the modes relevant to RPMh regulators.
Some RPMh regulators (BOB regulators only) also support bypass using
the standard "regulator-allow-bypass" binding.
...feel fee to reword, but basically the idea is to document it but
not make it look like you're defining a novel property.
>> Speaking of documenting things like that, it might be worth finding
>> somewhere in this doc to mention that the "bob" regulator on PMI8998
>> can support "regulator-allow-bypass". That tidbit got lost when we
>> moved to the standard regulator bindings for bypass.
>
> I suppose that I could add something like this:
>
> +- regulator-allow-bypass
> + Usage: optional; BOB type VRM regulators only
> + Value type: <empty>
> + Definition: See [2] for details.
> ...
> +[2]: Documentation/devicetree/bindings/regulator.txt
>
> However, I don't want the patch to get NACKed because it is defining a
> property that is already defined in the common regulator.txt file.
See above for my suggestion.
>>> +- qcom,allowed-drms-modes
>>> + Usage: required if regulator-allow-set-load is specified;
>>> + VRM regulators only
>>> + Value type: <prop-encoded-array>
>>> + Definition: A list of integers specifying the PMIC regulator modes which
>>> + can be configured at runtime based upon consumer load needs.
>>> + Supported values are RPMH_REGULATOR_MODE_* which are defined
>>> + in [1] (i.e. 0 to 3).
>>
>> Why is this still here? You moved it to the core regulator framework,
>> right? It's still in your examples too. Shouldn't this be removed?
>> It looks like the driver still needs this and it needs to be an exact
>> duplicate of the common binding. That doesn't seem right...
>
> The qcom,allowed-drms-modes property supports a different feature than the
> regulator-allowed-modes property accepted in [2]. The latter specifies
> the modes that may be used at all (e.g. in regulator_set_mode() calls) and
> it lists the mode values in an unordered fashion.
>
> qcom,allowed-drms-modes defines a specific subset of the possible allowed
> modes that should be set based on DRMS (e.g. in regulator_set_load()
> calls). Its values are listed in a specific order and must match 1-to-1
> with qcom,drms-mode-max-microamps entries.
>
> It would probably be good to change the name of the property from
> qcom,allowed-drms-modes to qcom,regulator-drms-modes.
Ah, I see. It's unfortunate that now we need to effectively list all
modes twice. Have you seen real-life examples where these sets of
modes need to be different, or is this just theoretical? If not can
we start with one property (that controls both things) and if we
really see that we need to specify different sets of modes for the two
cases we can add a separate property? ...actually, even if you do
have real-life examples of where these need to be different, if 90% of
the time they are the same it would still be nice to just have one
property apply to both cases.
>>> +- qcom,drms-mode-max-microamps
>>> + Usage: required if regulator-allow-set-load is specified;
>>> + VRM regulators only
>>> + Value type: <prop-encoded-array>
>>> + Definition: A list of integers specifying the maximum allowed load
>>> + current in microamps for each of the modes listed in
>>> + qcom,allowed-drms-modes (matched 1-to-1 in order). Elements
>>> + must be specified in order from lowest to highest value.
>>
>> Any reason this can't go into the regulator core? You'd basically
>> just take the existing concept of rpmh_regulator_vrm_set_load() and
>> put it in the core.
>
> This could be implemented in the core via new constraint elements parsed
> in of_regulator and a helper function to specify in regulator_ops.
> However, I'm not sure about the wide-spread applicability of this feature.
> I'd prefer to leave it in the driver unless Mark would like me to add it
> into the core.
You're already using pre-existing APIs around specifying the current
and having the regulator core call you to map the total current into a
mode. That implies that this is applicable to others. Adding this
tiny amount of code to the core makes the pre-existing APIs generally
useful.
>>> +- qcom,headroom-microvolt
>>> + Usage: optional; VRM regulators only
>>> + Value type: <u32>
>>> + Definition: Specifies the headroom voltage in microvolts to request for
>>> + a VRM regulator. RPMh hardware automatically ensures that
>>> + the parent of this regulator outputs a voltage high enough
>>> + to satisfy the requested headroom. Supported values are
>>> + 0 to 511000.
>>
>> I'm curious: is this a voted-for value, or a global value?
>>
>> Said another way: the whole point of RPMh is that there may be more
>> than one processor that needs the same rails, right? So the AP might
>> request 1.1 V for a rail and the modem might request 1.3 V. RPMh
>> would decide to pick the higher of those two (1.3 V), but if the modem
>> said it no longer needs the rail it will drop down to 1.1 V.
>>
>> ...and as an example of why the headroom needs to be in hardware, if
>> the source voltage was normally 1.4 V and the headroom was 200 mV then
>> the hardware would need to know to bump up the source voltage to 1.5V
>> during the period of of time that the modem wants the rail at 1.3V.
>>
>> So my question is: do the AP and modem in the above situation
>> separately vote for headroom? How is it aggregated? ...or is it a
>> global value and this sets the headroom for all clients of RPMh? It
>> would be interesting to document this as it might help with figuring
>> out how this value should be set.
>
> The headroom voltage voting is supported in hardware per-regulator and
> per-master (AP, modem, etc). The headroom voltage and output voltage are
> each aggregated (using max) per-regulator across masters. If the
> aggregated enable state for a regulator is on, then the aggregated output
> voltage and headroom voltage are added together and applied as a min
> constraint on the parent's output voltage (if there is a parent).
Ah, interesting. I'm not 100% convinced that the RPMh API is at the
right abstraction level here. I guess you increase the headroom
voltage if you expect a lot of current and need the regulator to still
give a clean signal? If you truly wanted to aggregate then if both
the modem and AP wanted to draw a lot of current they would both need
to increase the headroom and then the headroom should maybe not be the
max but something slightly more (you wouldn't want to add, but ...)
Since it's just a max, in theory it seems like you get 99% of the way
there by just using the Linux APIs to deal with dropout voltage. If
Linux was managing it in software then if it needed to account for
extra headroom it would just increase the supply voltage. That should
play just fine with the modem (which might be using the hardware
headroom feature) since it will be making its own completely separate
requests and they should be aggregated OK.
In another thread you said you'd be OK dropping the headroom voltage
since it wasn't needed on SDM845. Maybe we should do that? ...and if
someone later needs to account for a larger dropout they can figure
out how to hookup the standard linux min_dropout_uV?
^ permalink raw reply
* [PATCH V2 3/3] ARM: dts: imx7: correct enet ipg clock
From: Anson Huang @ 2018-05-18 1:01 UTC (permalink / raw)
To: shawnguo, kernel, fabio.estevam, robh+dt, mark.rutland,
mturquette, sboyd, stefan, adriana.reus, rui.silva
Cc: Linux-imx, linux-arm-kernel, devicetree, linux-kernel, linux-clk
In-Reply-To: <1526605266-18464-1-git-send-email-Anson.Huang@nxp.com>
ENET "ipg" clock should be IMX7D_ENETx_IPG_ROOT_CLK
rather than IMX7D_ENET_AXI_ROOT_CLK which is for ENET bus
clock.
Based on Andy Duan's patch from the NXP kernel tree.
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
arch/arm/boot/dts/imx7d.dtsi | 2 +-
arch/arm/boot/dts/imx7s.dtsi | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/dts/imx7d.dtsi b/arch/arm/boot/dts/imx7d.dtsi
index 200714e..d74dd7f 100644
--- a/arch/arm/boot/dts/imx7d.dtsi
+++ b/arch/arm/boot/dts/imx7d.dtsi
@@ -120,7 +120,7 @@
<GIC_SPI 100 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 101 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
- clocks = <&clks IMX7D_ENET_AXI_ROOT_CLK>,
+ clocks = <&clks IMX7D_ENET2_IPG_ROOT_CLK>,
<&clks IMX7D_ENET_AXI_ROOT_CLK>,
<&clks IMX7D_ENET2_TIME_ROOT_CLK>,
<&clks IMX7D_PLL_ENET_MAIN_125M_CLK>,
diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
index 4d42335..b90769d 100644
--- a/arch/arm/boot/dts/imx7s.dtsi
+++ b/arch/arm/boot/dts/imx7s.dtsi
@@ -1091,7 +1091,7 @@
<GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 119 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>;
- clocks = <&clks IMX7D_ENET_AXI_ROOT_CLK>,
+ clocks = <&clks IMX7D_ENET1_IPG_ROOT_CLK>,
<&clks IMX7D_ENET_AXI_ROOT_CLK>,
<&clks IMX7D_ENET1_TIME_ROOT_CLK>,
<&clks IMX7D_PLL_ENET_MAIN_125M_CLK>,
--
2.7.4
^ permalink raw reply related
* [PATCH V2 2/3] clk: imx7d: correct enet clock CCGR registers
From: Anson Huang @ 2018-05-18 1:01 UTC (permalink / raw)
To: shawnguo, kernel, fabio.estevam, robh+dt, mark.rutland,
mturquette, sboyd, stefan, adriana.reus, rui.silva
Cc: Linux-imx, linux-arm-kernel, devicetree, linux-kernel, linux-clk
In-Reply-To: <1526605266-18464-1-git-send-email-Anson.Huang@nxp.com>
Correct enet clock gates as below:
CCGR6: IMX7D_ENET_AXI_ROOT_CLK (enet1 and enet2 bus clocks)
CCGR112: IMX7D_ENET1_TIME_ROOT_CLK, IMX7D_ENET1_IPG_ROOT_CLK
CCGR113: IMX7D_ENET2_TIME_ROOT_CLK, IMX7D_ENET2_IPG_ROOT_CLK
Just rename unused IMX7D_ENETx_REF_ROOT_CLK for
IMX7D_ENETx_IPG_ROOT_CLK instead of adding new clocks.
Based on Andy Duan's patch from the NXP kernel tree.
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
drivers/clk/imx/clk-imx7d.c | 10 ++++++----
include/dt-bindings/clock/imx7d-clock.h | 4 ++--
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/clk/imx/clk-imx7d.c b/drivers/clk/imx/clk-imx7d.c
index 23d5090a..d4936b9 100644
--- a/drivers/clk/imx/clk-imx7d.c
+++ b/drivers/clk/imx/clk-imx7d.c
@@ -26,6 +26,8 @@ static u32 share_count_sai1;
static u32 share_count_sai2;
static u32 share_count_sai3;
static u32 share_count_nand;
+static u32 share_count_enet1;
+static u32 share_count_enet2;
static const struct clk_div_table test_div_table[] = {
{ .val = 3, .div = 1, },
@@ -805,6 +807,10 @@ static void __init imx7d_clocks_init(struct device_node *ccm_node)
clks[IMX7D_MIPI_DSI_ROOT_CLK] = imx_clk_gate4("mipi_dsi_root_clk", "mipi_dsi_post_div", base + 0x4650, 0);
clks[IMX7D_MIPI_CSI_ROOT_CLK] = imx_clk_gate4("mipi_csi_root_clk", "mipi_csi_post_div", base + 0x4640, 0);
clks[IMX7D_MIPI_DPHY_ROOT_CLK] = imx_clk_gate4("mipi_dphy_root_clk", "mipi_dphy_post_div", base + 0x4660, 0);
+ clks[IMX7D_ENET1_IPG_ROOT_CLK] = imx_clk_gate2_shared2("enet1_ipg_root_clk", "enet_axi_post_div", base + 0x4700, 0, &share_count_enet1);
+ clks[IMX7D_ENET1_TIME_ROOT_CLK] = imx_clk_gate2_shared2("enet1_time_root_clk", "enet1_time_post_div", base + 0x4700, 0, &share_count_enet1);
+ clks[IMX7D_ENET2_IPG_ROOT_CLK] = imx_clk_gate2_shared2("enet2_ipg_root_clk", "enet_axi_post_div", base + 0x4710, 0, &share_count_enet2);
+ clks[IMX7D_ENET2_TIME_ROOT_CLK] = imx_clk_gate2_shared2("enet2_time_root_clk", "enet2_time_post_div", base + 0x4710, 0, &share_count_enet2);
clks[IMX7D_SAI1_ROOT_CLK] = imx_clk_gate2_shared2("sai1_root_clk", "sai1_post_div", base + 0x48c0, 0, &share_count_sai1);
clks[IMX7D_SAI1_IPG_CLK] = imx_clk_gate2_shared2("sai1_ipg_clk", "ipg_root_clk", base + 0x48c0, 0, &share_count_sai1);
clks[IMX7D_SAI2_ROOT_CLK] = imx_clk_gate2_shared2("sai2_root_clk", "sai2_post_div", base + 0x48d0, 0, &share_count_sai2);
@@ -812,10 +818,6 @@ static void __init imx7d_clocks_init(struct device_node *ccm_node)
clks[IMX7D_SAI3_ROOT_CLK] = imx_clk_gate2_shared2("sai3_root_clk", "sai3_post_div", base + 0x48e0, 0, &share_count_sai3);
clks[IMX7D_SAI3_IPG_CLK] = imx_clk_gate2_shared2("sai3_ipg_clk", "ipg_root_clk", base + 0x48e0, 0, &share_count_sai3);
clks[IMX7D_SPDIF_ROOT_CLK] = imx_clk_gate4("spdif_root_clk", "spdif_post_div", base + 0x44d0, 0);
- clks[IMX7D_ENET1_REF_ROOT_CLK] = imx_clk_gate4("enet1_ref_root_clk", "enet1_ref_post_div", base + 0x44e0, 0);
- clks[IMX7D_ENET1_TIME_ROOT_CLK] = imx_clk_gate4("enet1_time_root_clk", "enet1_time_post_div", base + 0x44f0, 0);
- clks[IMX7D_ENET2_REF_ROOT_CLK] = imx_clk_gate4("enet2_ref_root_clk", "enet2_ref_post_div", base + 0x4500, 0);
- clks[IMX7D_ENET2_TIME_ROOT_CLK] = imx_clk_gate4("enet2_time_root_clk", "enet2_time_post_div", base + 0x4510, 0);
clks[IMX7D_EIM_ROOT_CLK] = imx_clk_gate4("eim_root_clk", "eim_post_div", base + 0x4160, 0);
clks[IMX7D_NAND_RAWNAND_CLK] = imx_clk_gate2_shared2("nand_rawnand_clk", "nand_root_clk", base + 0x4140, 0, &share_count_nand);
clks[IMX7D_NAND_USDHC_BUS_RAWNAND_CLK] = imx_clk_gate2_shared2("nand_usdhc_rawnand_clk", "nand_usdhc_root_clk", base + 0x4140, 0, &share_count_nand);
diff --git a/include/dt-bindings/clock/imx7d-clock.h b/include/dt-bindings/clock/imx7d-clock.h
index b2325d3e2..0d67f53 100644
--- a/include/dt-bindings/clock/imx7d-clock.h
+++ b/include/dt-bindings/clock/imx7d-clock.h
@@ -168,7 +168,7 @@
#define IMX7D_SPDIF_ROOT_SRC 155
#define IMX7D_SPDIF_ROOT_CG 156
#define IMX7D_SPDIF_ROOT_DIV 157
-#define IMX7D_ENET1_REF_ROOT_CLK 158
+#define IMX7D_ENET1_IPG_ROOT_CLK 158
#define IMX7D_ENET1_REF_ROOT_SRC 159
#define IMX7D_ENET1_REF_ROOT_CG 160
#define IMX7D_ENET1_REF_ROOT_DIV 161
@@ -176,7 +176,7 @@
#define IMX7D_ENET1_TIME_ROOT_SRC 163
#define IMX7D_ENET1_TIME_ROOT_CG 164
#define IMX7D_ENET1_TIME_ROOT_DIV 165
-#define IMX7D_ENET2_REF_ROOT_CLK 166
+#define IMX7D_ENET2_IPG_ROOT_CLK 166
#define IMX7D_ENET2_REF_ROOT_SRC 167
#define IMX7D_ENET2_REF_ROOT_CG 168
#define IMX7D_ENET2_REF_ROOT_DIV 169
--
2.7.4
^ permalink raw reply related
* [PATCH V2 1/3] clk: imx7d: correct enet phy ref clock gates
From: Anson Huang @ 2018-05-18 1:01 UTC (permalink / raw)
To: shawnguo, kernel, fabio.estevam, robh+dt, mark.rutland,
mturquette, sboyd, stefan, adriana.reus, rui.silva
Cc: Linux-imx, linux-arm-kernel, devicetree, linux-kernel, linux-clk
IMX7D_ENET_PHY_REF_ROOT_DIV supplies clock for PHY directly,
there is no clock gate after it, rename it to
IMX7D_ENET_PHY_REF_ROOT_CLK to avoid device tree change.
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
drivers/clk/imx/clk-imx7d.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/clk/imx/clk-imx7d.c b/drivers/clk/imx/clk-imx7d.c
index 975a20d..23d5090a 100644
--- a/drivers/clk/imx/clk-imx7d.c
+++ b/drivers/clk/imx/clk-imx7d.c
@@ -738,7 +738,7 @@ static void __init imx7d_clocks_init(struct device_node *ccm_node)
clks[IMX7D_ENET1_TIME_ROOT_DIV] = imx_clk_divider2("enet1_time_post_div", "enet1_time_pre_div", base + 0xa780, 0, 6);
clks[IMX7D_ENET2_REF_ROOT_DIV] = imx_clk_divider2("enet2_ref_post_div", "enet2_ref_pre_div", base + 0xa800, 0, 6);
clks[IMX7D_ENET2_TIME_ROOT_DIV] = imx_clk_divider2("enet2_time_post_div", "enet2_time_pre_div", base + 0xa880, 0, 6);
- clks[IMX7D_ENET_PHY_REF_ROOT_DIV] = imx_clk_divider2("enet_phy_ref_post_div", "enet_phy_ref_pre_div", base + 0xa900, 0, 6);
+ clks[IMX7D_ENET_PHY_REF_ROOT_CLK] = imx_clk_divider2("enet_phy_ref_root_clk", "enet_phy_ref_pre_div", base + 0xa900, 0, 6);
clks[IMX7D_EIM_ROOT_DIV] = imx_clk_divider2("eim_post_div", "eim_pre_div", base + 0xa980, 0, 6);
clks[IMX7D_NAND_ROOT_CLK] = imx_clk_divider2("nand_root_clk", "nand_pre_div", base + 0xaa00, 0, 6);
clks[IMX7D_QSPI_ROOT_DIV] = imx_clk_divider2("qspi_post_div", "qspi_pre_div", base + 0xaa80, 0, 6);
@@ -816,7 +816,6 @@ static void __init imx7d_clocks_init(struct device_node *ccm_node)
clks[IMX7D_ENET1_TIME_ROOT_CLK] = imx_clk_gate4("enet1_time_root_clk", "enet1_time_post_div", base + 0x44f0, 0);
clks[IMX7D_ENET2_REF_ROOT_CLK] = imx_clk_gate4("enet2_ref_root_clk", "enet2_ref_post_div", base + 0x4500, 0);
clks[IMX7D_ENET2_TIME_ROOT_CLK] = imx_clk_gate4("enet2_time_root_clk", "enet2_time_post_div", base + 0x4510, 0);
- clks[IMX7D_ENET_PHY_REF_ROOT_CLK] = imx_clk_gate4("enet_phy_ref_root_clk", "enet_phy_ref_post_div", base + 0x4520, 0);
clks[IMX7D_EIM_ROOT_CLK] = imx_clk_gate4("eim_root_clk", "eim_post_div", base + 0x4160, 0);
clks[IMX7D_NAND_RAWNAND_CLK] = imx_clk_gate2_shared2("nand_rawnand_clk", "nand_root_clk", base + 0x4140, 0, &share_count_nand);
clks[IMX7D_NAND_USDHC_BUS_RAWNAND_CLK] = imx_clk_gate2_shared2("nand_usdhc_rawnand_clk", "nand_usdhc_root_clk", base + 0x4140, 0, &share_count_nand);
--
2.7.4
^ permalink raw reply related
* [PATCH V5] ARM: dts: da850-evm: Enable LCD and Backlight
From: Adam Ford @ 2018-05-18 0:59 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: Adam Ford, devicetree, nsekhar, adam.ford
When using the board files the LCD works, but not with the DT.
This adds enables the original da850-evm to work with the same
LCD in device tree mode.
The EVM has a gpio for the regulator and a PWM for dimming the
backlight. The LCD and the vpif display pins are mutually
exclusive, so if using the LCD, do not load the vpif driver.
Signed-off-by: Adam Ford <aford173@gmail.com>
---
V5: Resync against v4.18/dt
V4: Move the backlight to PWM, so the driver can control the regulator allowing the
regulator to power down and enabling the ability to change the brightness of the
backlight
V3: Fix errant GPIO, label GPIO pins, and rename the regulator to be more explict to
backlight which better matches the schematic. Updated the description to explain
that it cannot be used at the same time as the vpif driver.
V2: Add regulator and GPIO enable pins. Remove PWM backlight and replace with GPIO
diff --git a/arch/arm/boot/dts/da850-evm.dts b/arch/arm/boot/dts/da850-evm.dts
index 0e82bb988fde..5bf6ea513b12 100644
--- a/arch/arm/boot/dts/da850-evm.dts
+++ b/arch/arm/boot/dts/da850-evm.dts
@@ -27,6 +27,58 @@
spi0 = &spi1;
};
+ backlight:backlight-pwm {
+ pinctrl-names = "default";
+ pinctrl-0 = <&ecap2_pins>;
+ power-supply = <&backlight_reg>;
+ compatible = "pwm-backlight";
+ pwms = <&ecap2 0 50000 0>;
+ brightness-levels = <0 10 20 30 40 50 60 70 80 90 99>;
+ default-brightness-level = <7>;
+ };
+
+ panel {
+ compatible = "ti,tilcdc,panel";
+ pinctrl-names = "default";
+ pinctrl-0 = <&lcd_pins>;
+ /* The vpif and the LCD are mutually exclusive.
+ * To enable VPIF, change the status below to 'disabled' then
+ * then change the status of the vpif below to 'okay' */
+ status = "okay";
+ enable-gpios = <&gpio 40 GPIO_ACTIVE_HIGH>; /* lcd_panel_pwr */
+
+ panel-info {
+ ac-bias = <255>;
+ ac-bias-intrpt = <0>;
+ dma-burst-sz = <16>;
+ bpp = <16>;
+ fdd = <0x80>;
+ sync-edge = <0>;
+ sync-ctrl = <1>;
+ raster-order = <0>;
+ fifo-th = <0>;
+ };
+
+ display-timings {
+ native-mode = <&timing0>;
+ timing0: 480x272 {
+ clock-frequency = <9000000>;
+ hactive = <480>;
+ vactive = <272>;
+ hfront-porch = <3>;
+ hback-porch = <2>;
+ hsync-len = <42>;
+ vback-porch = <3>;
+ vfront-porch = <4>;
+ vsync-len = <11>;
+ hsync-active = <0>;
+ vsync-active = <0>;
+ de-active = <1>;
+ pixelclk-active = <1>;
+ };
+ };
+ };
+
vbat: fixedregulator0 {
compatible = "regulator-fixed";
regulator-name = "vbat";
@@ -35,6 +87,15 @@
regulator-boot-on;
};
+ backlight_reg: backlight-regulator {
+ compatible = "regulator-fixed";
+ regulator-name = "lcd_backlight_pwr";
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ gpio = <&gpio 47 GPIO_ACTIVE_HIGH>; /* lcd_backlight_pwr */
+ enable-active-high;
+ };
+
sound {
compatible = "simple-audio-card";
simple-audio-card,name = "DA850/OMAP-L138 EVM";
@@ -63,6 +124,10 @@
};
};
+&ecap2 {
+ status = "okay";
+};
+
&pmx_core {
status = "okay";
@@ -109,6 +174,10 @@
status = "okay";
};
+&lcdc {
+ status = "okay";
+};
+
&i2c0 {
status = "okay";
clock-frequency = <100000>;
@@ -336,5 +405,8 @@
&vpif {
pinctrl-names = "default";
pinctrl-0 = <&vpif_capture_pins>, <&vpif_display_pins>;
- status = "okay";
+ /* The vpif and the LCD are mutually exclusive.
+ * To enable VPIF, disable the ti,tilcdc,panel then
+ * changed the status below to 'okay' */
+ status = "disabled";
};
--
2.17.0
^ permalink raw reply related
* [PATCH V4] ARM: dts: da850-evm: Enable LCD and Backlight
From: Adam Ford @ 2018-05-18 0:49 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: Adam Ford, devicetree, nsekhar, adam.ford
When using the board files the LCD works, but not with the DT.
This adds enables the original da850-evm to work with the same
LCD in device tree mode.
The EVM has a gpio for the regulator and a PWM for dimming the
backlight. The LCD and the vpif display pins are mutually
exclusive, so if using the LCD, do not load the vpif driver.
Signed-off-by: Adam Ford <aford173@gmail.com>
---
V4: Move the backlight to PWM, so the driver can control the regulator allowing the
regulator to power down and enabling the ability to change the brightness of the
backlight
V3: Fix errant GPIO, label GPIO pins, and rename the regulator to be more explict to
backlight which better matches the schematic. Updated the description to explain
that it cannot be used at the same time as the vpif driver.
V2: Add regulator and GPIO enable pins. Remove PWM backlight and replace with GPIO
diff --git a/arch/arm/boot/dts/da850-evm.dts b/arch/arm/boot/dts/da850-evm.dts
index f6a5497d9c97..7be31372bbc2 100644
--- a/arch/arm/boot/dts/da850-evm.dts
+++ b/arch/arm/boot/dts/da850-evm.dts
@@ -27,9 +27,14 @@
spi0 = &spi1;
};
- backlight {
- compatible = "gpio-backlight";
- gpios = <&gpio 7 GPIO_ACTIVE_HIGH>; /* lcd_pwm0 */
+ backlight:backlight-pwm {
+ pinctrl-names = "default";
+ pinctrl-0 = <&ecap2_pins>;
+ power-supply = <&backlight_reg>;
+ compatible = "pwm-backlight";
+ pwms = <&ecap2 0 50000 0>;
+ brightness-levels = <0 10 20 30 40 50 60 70 80 90 99>;
+ default-brightness-level = <7>;
};
panel {
@@ -88,7 +93,6 @@
regulator-min-microvolt = <3300000>;
regulator-max-microvolt = <3300000>;
gpio = <&gpio 47 GPIO_ACTIVE_HIGH>; /* lcd_backlight_pwr */
- regulator-always-on;
enable-active-high;
};
@@ -120,6 +124,10 @@
};
};
+&ecap2 {
+ status = "okay";
+};
+
&pmx_core {
status = "okay";
--
2.17.0
^ permalink raw reply related
* Re: [PATCH v3 2/2] regulator: add QCOM RPMh regulator driver
From: David Collins @ 2018-05-18 0:16 UTC (permalink / raw)
To: Doug Anderson
Cc: Mark Brown, Liam Girdwood, Rob Herring, Mark Rutland,
linux-arm-msm, Linux ARM, devicetree, LKML, Rajendra Nayak,
Stephen Boyd
In-Reply-To: <CAD=FV=UiZEFTAtO9C0UbRO=ow5=dv6zNG-XMqhHm1Fmp2GwVcA@mail.gmail.com>
On 05/17/2018 02:23 PM, Doug Anderson wrote:
> On Fri, May 11, 2018 at 7:28 PM, David Collins <collinsd@codeaurora.org> wrote:
>> + /*
>> + * Default the voltage selector to an error value in the
>> + * case that qcom,regulator-initial-microvolt is not
>> + * specified in device tree since the true voltage is
>> + * not known. Note that this value causes
>> + * devm_regulator_register() to fail in the case that
>> + * regulator-min-microvolt and regulator-max-microvolt
>> + * are specified in device tree due to
>> + * machine_constraints_voltage() bailing when the
>> + * get_voltage_sel() callback returns this error value.
>> + */
>> + vreg->voltage_selector = -EINVAL;
>
> As per comments in other threads, adjust this comment and use
> -ENOTRECOVERABLE now.
I'll make this change.
Take care,
David
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings
From: David Collins @ 2018-05-18 0:16 UTC (permalink / raw)
To: Doug Anderson
Cc: Mark Brown, Liam Girdwood, Rob Herring, Mark Rutland,
linux-arm-msm, Linux ARM, devicetree, LKML, Rajendra Nayak,
Stephen Boyd
In-Reply-To: <CAD=FV=XegPeFR8pdZM-YiBBJjrbej=C+2biBt4zJKnE90uTYZA@mail.gmail.com>
On 05/17/2018 02:22 PM, Doug Anderson wrote:
> On Fri, May 11, 2018 at 7:28 PM, David Collins <collinsd@codeaurora.org> wrote:
>> +- qcom,regulator-initial-microvolt
>> + Usage: optional; VRM regulators only
>> + Value type: <u32>
>> + Definition: Specifies the initial voltage in microvolts to request for a
>> + VRM regulator.
>
> Now that Mark has landed the patch adding support for the
> -ENOTRECOVERABLE error code from get_voltage() / get_voltage_sel(), do
> we still need the qcom,regulator-initial-microvolt property?
Yes, this is still needed. The -ENOTRECOVERABLE patch ensures that
qcom-rpmh-regulator devices can be registered even if
qcom,regulator-initial-microvolt is not specified. However, that will
result in the regulators being configured for the minimum voltage
supported in the DT specified min/max range. The
qcom,regulator-initial-microvolt property allows us to set a specific
voltage that is larger than the min constraint.
> If this is really still needed, can it be moved to the regulator core?
I'm not opposed to the idea, but I think that Mark is [1]:
>> Do you have a preference for qcom,regulator-initial-microvolt vs a generic
>> framework supported regulator-initial-microvolt property for configuring a
>> specific voltage at registration time? We'll need to have support for one
>> or the other in order for the qcom_rpmh-regulator driver to be functional.
>
> This is basically specific to Qualcomm, I can't off hand think of any
> other devices with similar issues.
>> +- regulator-initial-mode
>> + Usage: optional; VRM regulators only
>> + Value type: <u32>
>> + Definition: Specifies the initial mode to request for a VRM regulator.
>> + Supported values are RPMH_REGULATOR_MODE_* which are defined
>> + in [1] (i.e. 0 to 3). This property may be specified even
>> + if the regulator-allow-set-load property is not specified.
>
> Every time I read the above I wonder why you're documenting a standard
> regulator regulator property in your bindings. ...then I realize it's
> because you're doing it because you want to explicitly document what
> the valid modes are. I wonder if it makes sense to just put a
> reference somewhere else in this document to go look at the header
> file where these are all nicely documented.
Isn't that what the [1] in the above snippet is currently doing. Further
down in qcom,rpmh-regulator.txt is this line:
+[1] include/dt-bindings/regulator/qcom,rpmh-regulator.h
> Speaking of documenting things like that, it might be worth finding
> somewhere in this doc to mention that the "bob" regulator on PMI8998
> can support "regulator-allow-bypass". That tidbit got lost when we
> moved to the standard regulator bindings for bypass.
I suppose that I could add something like this:
+- regulator-allow-bypass
+ Usage: optional; BOB type VRM regulators only
+ Value type: <empty>
+ Definition: See [2] for details.
...
+[2]: Documentation/devicetree/bindings/regulator.txt
However, I don't want the patch to get NACKed because it is defining a
property that is already defined in the common regulator.txt file.
>> +- qcom,allowed-drms-modes
>> + Usage: required if regulator-allow-set-load is specified;
>> + VRM regulators only
>> + Value type: <prop-encoded-array>
>> + Definition: A list of integers specifying the PMIC regulator modes which
>> + can be configured at runtime based upon consumer load needs.
>> + Supported values are RPMH_REGULATOR_MODE_* which are defined
>> + in [1] (i.e. 0 to 3).
>
> Why is this still here? You moved it to the core regulator framework,
> right? It's still in your examples too. Shouldn't this be removed?
> It looks like the driver still needs this and it needs to be an exact
> duplicate of the common binding. That doesn't seem right...
The qcom,allowed-drms-modes property supports a different feature than the
regulator-allowed-modes property accepted in [2]. The latter specifies
the modes that may be used at all (e.g. in regulator_set_mode() calls) and
it lists the mode values in an unordered fashion.
qcom,allowed-drms-modes defines a specific subset of the possible allowed
modes that should be set based on DRMS (e.g. in regulator_set_load()
calls). Its values are listed in a specific order and must match 1-to-1
with qcom,drms-mode-max-microamps entries.
It would probably be good to change the name of the property from
qcom,allowed-drms-modes to qcom,regulator-drms-modes.
>> +- qcom,drms-mode-max-microamps
>> + Usage: required if regulator-allow-set-load is specified;
>> + VRM regulators only
>> + Value type: <prop-encoded-array>
>> + Definition: A list of integers specifying the maximum allowed load
>> + current in microamps for each of the modes listed in
>> + qcom,allowed-drms-modes (matched 1-to-1 in order). Elements
>> + must be specified in order from lowest to highest value.
>
> Any reason this can't go into the regulator core? You'd basically
> just take the existing concept of rpmh_regulator_vrm_set_load() and
> put it in the core.
This could be implemented in the core via new constraint elements parsed
in of_regulator and a helper function to specify in regulator_ops.
However, I'm not sure about the wide-spread applicability of this feature.
I'd prefer to leave it in the driver unless Mark would like me to add it
into the core.
>> +- qcom,headroom-microvolt
>> + Usage: optional; VRM regulators only
>> + Value type: <u32>
>> + Definition: Specifies the headroom voltage in microvolts to request for
>> + a VRM regulator. RPMh hardware automatically ensures that
>> + the parent of this regulator outputs a voltage high enough
>> + to satisfy the requested headroom. Supported values are
>> + 0 to 511000.
>
> I'm curious: is this a voted-for value, or a global value?
>
> Said another way: the whole point of RPMh is that there may be more
> than one processor that needs the same rails, right? So the AP might
> request 1.1 V for a rail and the modem might request 1.3 V. RPMh
> would decide to pick the higher of those two (1.3 V), but if the modem
> said it no longer needs the rail it will drop down to 1.1 V.
>
> ...and as an example of why the headroom needs to be in hardware, if
> the source voltage was normally 1.4 V and the headroom was 200 mV then
> the hardware would need to know to bump up the source voltage to 1.5V
> during the period of of time that the modem wants the rail at 1.3V.
>
> So my question is: do the AP and modem in the above situation
> separately vote for headroom? How is it aggregated? ...or is it a
> global value and this sets the headroom for all clients of RPMh? It
> would be interesting to document this as it might help with figuring
> out how this value should be set.
The headroom voltage voting is supported in hardware per-regulator and
per-master (AP, modem, etc). The headroom voltage and output voltage are
each aggregated (using max) per-regulator across masters. If the
aggregated enable state for a regulator is on, then the aggregated output
voltage and headroom voltage are added together and applied as a min
constraint on the parent's output voltage (if there is a parent).
>> diff --git a/include/dt-bindings/regulator/qcom,rpmh-regulator.h b/include/dt-bindings/regulator/qcom,rpmh-regulator.h
>> new file mode 100644
>> index 0000000..4378c4b
>> --- /dev/null
>> +++ b/include/dt-bindings/regulator/qcom,rpmh-regulator.h
>> +/*
>> + * These mode constants may be used for regulator-initial-mode and
>> + * qcom,allowed-drms-modes properties of an RPMh regulator device tree node.
>
> Technically also for your new "regulator-allowed-modes". Maybe just
> say that they're used anywhere a regulator mode is needed in this
> driver and give regulator-initial-mode as an example?
Sure, I'll update this description.
Take care,
David
[1]: https://lkml.org/lkml/2018/4/24/960
[2]: https://lkml.org/lkml/2018/5/11/696
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v7 2/2] drivers: soc: Add LLCC driver
From: Evan Green @ 2018-05-17 22:30 UTC (permalink / raw)
To: rishabhb
Cc: linux-arm-kernel, linux-arm-msm, devicetree, linux-kernel,
linux-arm, tsoni, ckadabi, robh
In-Reply-To: <1526492623-20527-3-git-send-email-rishabhb@codeaurora.org>
On Wed, May 16, 2018 at 10:44 AM Rishabh Bhatnagar <rishabhb@codeaurora.org>
wrote:
> LLCC (Last Level Cache Controller) provides additional cache memory
> in the system. LLCC is partitioned into multiple slices and each
> slice gets its own priority, size, ID and other config parameters.
> LLCC driver programs these parameters for each slice. Clients that
> are assigned to use LLCC need to get information such size & ID of the
> slice they get and activate or deactivate the slice as needed. LLCC driver
> provides API for the clients to perform these operations.
> Signed-off-by: Channagoud Kadabi <ckadabi@codeaurora.org>
> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> ---
> drivers/soc/qcom/Kconfig | 17 ++
> drivers/soc/qcom/Makefile | 2 +
> drivers/soc/qcom/llcc-sdm845.c | 106 ++++++++++++
> drivers/soc/qcom/llcc-slice.c | 337
+++++++++++++++++++++++++++++++++++++
> include/linux/soc/qcom/llcc-qcom.h | 180 ++++++++++++++++++++
> 5 files changed, 642 insertions(+)
> create mode 100644 drivers/soc/qcom/llcc-sdm845.c
> create mode 100644 drivers/soc/qcom/llcc-slice.c
> create mode 100644 include/linux/soc/qcom/llcc-qcom.h
Thanks Rishabh.
Reviewed-by: Evan Green <evgreen@chromium.org>
^ permalink raw reply
* Re: [PATCH v6 2/2] leds: lm3601x: Introduce the lm3601x LED driver
From: Jacek Anaszewski @ 2018-05-17 21:26 UTC (permalink / raw)
To: Dan Murphy, Andy Shevchenko
Cc: Rob Herring, Mark Rutland, Pavel Machek, devicetree,
Linux Kernel Mailing List, Linux LED Subsystem
In-Reply-To: <54dcb6cc-7765-f64c-33fd-2920f3865153@ti.com>
Dan,
On 05/17/2018 04:34 PM, Dan Murphy wrote:
> Jacek
>
> On 05/16/2018 04:17 PM, Dan Murphy wrote:
> <snip>
>
>>>>>
>>>>>
>>>>>>>> + if (!ret)
>>>>>>>
>>>>>>> if (ret) sounds more natural. And better just to split
>>>>>>>
>>>>>>>> + snprintf(led->led_name, sizeof(led->led_name),
>>>>>>>> + "%s:%s", led->led_node->name, name);
>>>>>>>> + else
>>>>>>>> + snprintf(led->led_name, sizeof(led->led_name),
>>>>>>>> + "%s:torch", led->led_node->name);
>>>>>>>
>>>>>>> const char *tmp;
>>>>>>>
>>>>>>> ret = device_property_read_...(&tmp);
>>>>>>> if (ret)
>>>>>>> tmp = ...
>>>>>>> sprintf(...);
>>>>
>>>> We're no longer taking devicename section of a LED class device name
>>>> from DT, so it will look differently anyway.
>>>>
>
> So in adding the device_property code I think we again are reaching the LED label issue.
> In ARM with DT we would take the parent device node name and append it to the label
AFAIR this approach (parent DT node name used for devicename ) was
incidentally applied in leds-as3645a.c. Soon after that we started
to use led-controller for parent DT name, according to Rob's request.
In the most of LED class drivers this is a child DT node which is used
for devicename, in case label is absent.
> if the optional label property was not available. In migrating to the device_property
> APIs we don't or can't depend on that parent node anymore.
>
> So for the case where the label property does not exist should we use a hard coded name
> or should we try to use the name from a device_id table.
>
> This is how we did this for the leds-lp8860 driver. If the label did not exist we used the
> i2c_device_id table and pulled the string from there.
i2c_device_id can't be applied as a generic pattern, but only for I2C
hooked devices. Nonetheless since it allows to save few lines of code
in case of drivers supporting a family of chips we can use it.
We are going to get rid of a devicename section from LED class device
name soon anyway, since it is redundant.
--
Best regards,
Jacek Anaszewski
^ permalink raw reply
* Re: [PATCH v3 2/2] regulator: add QCOM RPMh regulator driver
From: Doug Anderson @ 2018-05-17 21:23 UTC (permalink / raw)
To: David Collins
Cc: Mark Brown, Liam Girdwood, Rob Herring, Mark Rutland,
linux-arm-msm, Linux ARM, devicetree, LKML, Rajendra Nayak,
Stephen Boyd
In-Reply-To: <3a4195365ab0d252fdf064d2300f45b9b777991c.1526088081.git.collinsd@codeaurora.org>
Hi,
On Fri, May 11, 2018 at 7:28 PM, David Collins <collinsd@codeaurora.org> wrote:
> +static int rpmh_regulator_parse_vrm_modes(struct rpmh_vreg *vreg,
> + struct device *dev, struct device_node *node)
> +{
> + const char *prop;
> + int i, len, ret, mode;
> + u32 *buf;
> +
> + /* qcom,allowed-drms-modes is optional */
> + prop = "qcom,allowed-drms-modes";
As per comments in bindings patch: this is a duplicate of your new
attribute you added to the regulator core. Makes no sense to have a
private attribute too with the same value.
> + prop = "qcom,drms-mode-max-microamps";
As per comments in the bindings patch, I think we should move
"qcom,drms-mode-max-microamps" to the regulator core.
> + prop = "qcom,regulator-initial-microvolt";
As per comments in bindings patch: seems like we should get rid of
"qcom,regulator-initial-microvolt" or move to the core.
> + /*
> + * Default the voltage selector to an error value in the
> + * case that qcom,regulator-initial-microvolt is not
> + * specified in device tree since the true voltage is
> + * not known. Note that this value causes
> + * devm_regulator_register() to fail in the case that
> + * regulator-min-microvolt and regulator-max-microvolt
> + * are specified in device tree due to
> + * machine_constraints_voltage() bailing when the
> + * get_voltage_sel() callback returns this error value.
> + */
> + vreg->voltage_selector = -EINVAL;
As per comments in other threads, adjust this comment and use
-ENOTRECOVERABLE now.
NOTE: I think this driver is looking really good now. Hopefully the
above things should be quick to spin (even getting "max-microamps" in
the core should be quick I think) and we can get something landed!
:)
-Doug
^ permalink raw reply
* Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings
From: Doug Anderson @ 2018-05-17 21:22 UTC (permalink / raw)
To: David Collins
Cc: Mark Brown, Liam Girdwood, Rob Herring, Mark Rutland,
linux-arm-msm, Linux ARM, devicetree, LKML, Rajendra Nayak,
Stephen Boyd
In-Reply-To: <41d5df73ddac772551d2966e0752bb0c357b1ded.1526088081.git.collinsd@codeaurora.org>
Hi,
On Fri, May 11, 2018 at 7:28 PM, David Collins <collinsd@codeaurora.org> wrote:
> +- qcom,regulator-initial-microvolt
> + Usage: optional; VRM regulators only
> + Value type: <u32>
> + Definition: Specifies the initial voltage in microvolts to request for a
> + VRM regulator.
Now that Mark has landed the patch adding support for the
-ENOTRECOVERABLE error code from get_voltage() / get_voltage_sel(), do
we still need the qcom,regulator-initial-microvolt property? If this
is really still needed, can it be moved to the regulator core?
> +- regulator-initial-mode
> + Usage: optional; VRM regulators only
> + Value type: <u32>
> + Definition: Specifies the initial mode to request for a VRM regulator.
> + Supported values are RPMH_REGULATOR_MODE_* which are defined
> + in [1] (i.e. 0 to 3). This property may be specified even
> + if the regulator-allow-set-load property is not specified.
Every time I read the above I wonder why you're documenting a standard
regulator regulator property in your bindings. ...then I realize it's
because you're doing it because you want to explicitly document what
the valid modes are. I wonder if it makes sense to just put a
reference somewhere else in this document to go look at the header
file where these are all nicely documented.
Speaking of documenting things like that, it might be worth finding
somewhere in this doc to mention that the "bob" regulator on PMI8998
can support "regulator-allow-bypass". That tidbit got lost when we
moved to the standard regulator bindings for bypass.
> +- qcom,allowed-drms-modes
> + Usage: required if regulator-allow-set-load is specified;
> + VRM regulators only
> + Value type: <prop-encoded-array>
> + Definition: A list of integers specifying the PMIC regulator modes which
> + can be configured at runtime based upon consumer load needs.
> + Supported values are RPMH_REGULATOR_MODE_* which are defined
> + in [1] (i.e. 0 to 3).
Why is this still here? You moved it to the core regulator framework,
right? It's still in your examples too. Shouldn't this be removed?
It looks like the driver still needs this and it needs to be an exact
duplicate of the common binding. That doesn't seem right...
> +- qcom,drms-mode-max-microamps
> + Usage: required if regulator-allow-set-load is specified;
> + VRM regulators only
> + Value type: <prop-encoded-array>
> + Definition: A list of integers specifying the maximum allowed load
> + current in microamps for each of the modes listed in
> + qcom,allowed-drms-modes (matched 1-to-1 in order). Elements
> + must be specified in order from lowest to highest value.
Any reason this can't go into the regulator core? You'd basically
just take the existing concept of rpmh_regulator_vrm_set_load() and
put it in the core.
> +- qcom,headroom-microvolt
> + Usage: optional; VRM regulators only
> + Value type: <u32>
> + Definition: Specifies the headroom voltage in microvolts to request for
> + a VRM regulator. RPMh hardware automatically ensures that
> + the parent of this regulator outputs a voltage high enough
> + to satisfy the requested headroom. Supported values are
> + 0 to 511000.
I'm curious: is this a voted-for value, or a global value?
Said another way: the whole point of RPMh is that there may be more
than one processor that needs the same rails, right? So the AP might
request 1.1 V for a rail and the modem might request 1.3 V. RPMh
would decide to pick the higher of those two (1.3 V), but if the modem
said it no longer needs the rail it will drop down to 1.1 V.
...and as an example of why the headroom needs to be in hardware, if
the source voltage was normally 1.4 V and the headroom was 200 mV then
the hardware would need to know to bump up the source voltage to 1.5V
during the period of of time that the modem wants the rail at 1.3V.
So my question is: do the AP and modem in the above situation
separately vote for headroom? How is it aggregated? ...or is it a
global value and this sets the headroom for all clients of RPMh? It
would be interesting to document this as it might help with figuring
out how this value should be set.
> diff --git a/include/dt-bindings/regulator/qcom,rpmh-regulator.h b/include/dt-bindings/regulator/qcom,rpmh-regulator.h
> new file mode 100644
> index 0000000..4378c4b
> --- /dev/null
> +++ b/include/dt-bindings/regulator/qcom,rpmh-regulator.h
> +/*
> + * These mode constants may be used for regulator-initial-mode and
> + * qcom,allowed-drms-modes properties of an RPMh regulator device tree node.
Technically also for your new "regulator-allowed-modes". Maybe just
say that they're used anywhere a regulator mode is needed in this
driver and give regulator-initial-mode as an example?
-Doug
^ permalink raw reply
* Re: [PATCH 2/2] regulator: of: add support for allowed modes configuration
From: Doug Anderson @ 2018-05-17 21:22 UTC (permalink / raw)
To: David Collins
Cc: Mark Brown, Liam Girdwood, Rob Herring, Mark Rutland,
linux-arm-msm, Linux ARM, devicetree, LKML, Rajendra Nayak,
Stephen Boyd
In-Reply-To: <58bb4f965515a67a6cbbc726b0d7b092c22b79d7.1526088289.git.collinsd@codeaurora.org>
Hi,
On Fri, May 11, 2018 at 6:46 PM, David Collins <collinsd@codeaurora.org> wrote:
> @@ -136,6 +136,33 @@ static void of_get_regulation_constraints(struct device_node *np,
> }
> }
>
> + len = of_property_count_elems_of_size(np, "regulator-allowed-modes",
> + sizeof(u32));
> + if (len > 0) {
> + if (desc && desc->of_map_mode) {
> + for (i = 0; i < len; i++) {
> + ret = of_property_read_u32_index(np,
> + "regulator-allowed-modes", i, &pval);
> + if (ret) {
> + pr_err("%s: couldn't read allowed modes index %d, ret=%d\n",
> + np->name, i, ret);
> + break;
> + }
> + mode = desc->of_map_mode(pval);
> + if (mode == REGULATOR_MODE_INVALID)
> + pr_err("%s: invalid regulator-allowed-modes element %u\n",
> + np->name, pval);
> + else
> + constraints->valid_modes_mask |= mode;
> + }
> + if (constraints->valid_modes_mask)
> + constraints->valid_ops_mask
> + |= REGULATOR_CHANGE_MODE;
Kinda calls into question the value of REGULATOR_CHANGE_MODE in the
valid_ops_mask if it's just set whenever valid_modes_mask is non-zero,
huh?
> + } else {
> + pr_warn("%s: mode mapping not defined\n", np->name);
> + }
> + }
> +
This patch seems good to me.
Reviewed-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply
* Re: [PATCH 1/2] regulator: of: add property for allowed modes specification
From: Doug Anderson @ 2018-05-17 21:21 UTC (permalink / raw)
To: David Collins
Cc: Mark Brown, Liam Girdwood, Rob Herring, Mark Rutland,
linux-arm-msm, Linux ARM, devicetree, LKML, Rajendra Nayak,
Stephen Boyd
In-Reply-To: <f3aa166ddd11c8f9813602be68aa5b10e5c90979.1526088289.git.collinsd@codeaurora.org>
Hi,
On Fri, May 11, 2018 at 6:46 PM, David Collins <collinsd@codeaurora.org> wrote:
> Add a common device tree property for regulator nodes to support
> the specification of allowed operating modes.
>
> Signed-off-by: David Collins <collinsd@codeaurora.org>
> ---
> Documentation/devicetree/bindings/regulator/regulator.txt | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt b/Documentation/devicetree/bindings/regulator/regulator.txt
> index 2babe15b..c627aa0 100644
> --- a/Documentation/devicetree/bindings/regulator/regulator.txt
> +++ b/Documentation/devicetree/bindings/regulator/regulator.txt
> @@ -59,6 +59,11 @@ Optional properties:
> - regulator-initial-mode: initial operating mode. The set of possible operating
> modes depends on the capabilities of every hardware so each device binding
> documentation explains which values the regulator supports.
> +- regulator-allowed-modes: list of operating modes that software is allowed to
> + configure for the regulator at run-time. Elements may be specified in any
> + order. The set of possible operating modes depends on the capabilities of
> + every hardware so each device binding document explains which values the
> + regulator supports.
Looks sane to me. It might be interesting to be explicit about what
happens if "regulator-allowed-modes" doesn't include the mode that was
listed as "regulator-initial-mode". Does that mean that there's no
way to get back to "regulator-initial-mode" after it's been changed
once, or is it an error to not include the initial mode in the set of
allowed modes?
I'm not 100% sure if going to such detail is necessary though. Thus,
feel free to add:
Reviewed-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply
* RE: [PATCH 1/2] dt-bindings: power: Add ZynqMP power domain bindings
From: Jolly Shah @ 2018-05-17 21:10 UTC (permalink / raw)
To: Jolly Shah, Marek Szyprowski, Geert Uytterhoeven, Rob Herring
Cc: Matthias Brugger, Andy Gross, Shawn Guo, Geert Uytterhoeven,
Björn Andersson, sean.wang@mediatek.com, Michal Simek,
Mark Rutland, Rajan Vaja,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List
In-Reply-To: <DM2PR0201MB07670C06F504F97EF4E4BAD4B8D00@DM2PR0201MB0767.namprd02.prod.outlook.com>
Hi Marek,
> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> owner@vger.kernel.org] On Behalf Of Jolly Shah
> Sent: Thursday, March 15, 2018 10:47 AM
> To: Marek Szyprowski <m.szyprowski@samsung.com>; Geert Uytterhoeven
> <geert@linux-m68k.org>; Rob Herring <robh@kernel.org>
> Cc: Matthias Brugger <matthias.bgg@gmail.com>; Andy Gross
> <andy.gross@linaro.org>; Shawn Guo <shawnguo@kernel.org>; Geert
> Uytterhoeven <geert+renesas@glider.be>; Björn Andersson
> <bjorn.andersson@linaro.org>; sean.wang@mediatek.com; Michal Simek
> <michal.simek@xilinx.com>; Mark Rutland <mark.rutland@arm.com>; Rajan
> Vaja <RAJANV@xilinx.com>; open list:OPEN FIRMWARE AND FLATTENED
> DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; Linux ARM <linux-arm-
> kernel@lists.infradead.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>
> Subject: RE: [PATCH 1/2] dt-bindings: power: Add ZynqMP power domain
> bindings
>
> [This sender failed our fraud detection checks and may not be who they appear
> to be. Learn about spoofing at http://aka.ms/LearnAboutSpoofing]
>
> Hi Rob, Geert, Marek,
>
> > -----Original Message-----
> > From: Marek Szyprowski [mailto:m.szyprowski@samsung.com]
> > Sent: Tuesday, March 06, 2018 12:06 AM
> > To: Geert Uytterhoeven <geert@linux-m68k.org>; Rob Herring
> > <robh@kernel.org>
> > Cc: Jolly Shah <JOLLYS@xilinx.com>; Matthias Brugger
> > <matthias.bgg@gmail.com>; Andy Gross <andy.gross@linaro.org>; Shawn
> > Guo <shawnguo@kernel.org>; Geert Uytterhoeven
> > <geert+renesas@glider.be>; Björn Andersson
> > <bjorn.andersson@linaro.org>; sean.wang@mediatek.com; Michal Simek
> > <michal.simek@xilinx.com>; Mark Rutland <mark.rutland@arm.com>; Rajan
> > Vaja <RAJANV@xilinx.com>; open list:OPEN FIRMWARE AND FLATTENED
> DEVICE
> > TREE BINDINGS <devicetree@vger.kernel.org>; Linux ARM <linux-arm-
> > kernel@lists.infradead.org>; Linux Kernel Mailing List <linux-
> > kernel@vger.kernel.org>; Jolly Shah <JOLLYS@xilinx.com>
> > Subject: Re: [PATCH 1/2] dt-bindings: power: Add ZynqMP power domain
> > bindings
> >
> > Hi All,
> >
> > On 2018-03-06 08:59, Geert Uytterhoeven wrote:
> > > Hi Rob, Jolly,
> > >
> > > On Mon, Mar 5, 2018 at 11:39 PM, Rob Herring <robh@kernel.org> wrote:
> > >> On Tue, Feb 27, 2018 at 03:55:49PM -0800, Jolly Shah wrote:
> > >>> Add documentation to describe ZynqMP power domain bindings.
> > >>>
> > >>> Signed-off-by: Jolly Shah <jollys@xilinx.com>
> > >>> Signed-off-by: Rajan Vaja <rajanv@xilinx.com>
> > >>> ---
> > >>> .../devicetree/bindings/power/zynqmp-genpd.txt | 46
> > ++++++++++++++++++++++
> > >>> 1 file changed, 46 insertions(+)
> > >>> create mode 100644
> > >>> Documentation/devicetree/bindings/power/zynqmp-genpd.txt
> > >>>
> > >>> diff --git
> > >>> a/Documentation/devicetree/bindings/power/zynqmp-genpd.txt
> > >>> b/Documentation/devicetree/bindings/power/zynqmp-genpd.txt
> > >>> new file mode 100644
> > >>> index 0000000..25f9711
> > >>> --- /dev/null
> > >>> +++ b/Documentation/devicetree/bindings/power/zynqmp-genpd.txt
> > >>> +This node contains a number of subnodes, each representing a
> > >>> +single PM domain that PM domain consumer devices reference.
> > >>> +
> > >>> +== PM Domain Nodes ==
> > >>> +
> > >>> +Required properties:
> > >>> + - #power-domain-cells: Number of cells in a PM domain specifier.
> > >>> +Must be
> > 0.
> > >>> + - pd-id: List of domain identifiers of as defined by platform firmware.
> > These
> > >>> + identifiers are passed to the PM firmware.
> > >>> +
> > >>> +Example:
> > >>> + zynqmp-genpd {
> > >>> + compatible = "xlnx,zynqmp-genpd";
> > >> What's the control interface for controlling the domains?
> > >>> +
> > >>> + pd_usb0: pd-usb0 {
> > >>> + pd-id = <22>;
> > >>> + #power-domain-cells = <0>;
> > >> There's no need for all these sub nodes. Make #power-domain-cells 1
> > >> and put the id in the cell value.
> > > That was my first reaction, too...
> > >>> + };
> > >>> +
> > >>> + pd_sata: pd-sata {
> > >>> + pd-id = <28>;
> > >>> + #power-domain-cells = <0>;
> > >>> + };
> > >>> +
> > >>> + pd_gpu: pd-gpu {
> > >>> + pd-id = <58 20 21>;
> > > ... until I saw the above.
> > > Controlling the GPU power area requires controlling 3 physical areas?
> > >
> > > However, doing it this way may bite you in the future, if a need
> > > arises to control a subset. And what about power up/down order?
> >
> > What about defining 3 separate domains and arranging them in
> > parent-child relationship? generic power domains already supports that
> > and this allows to nicely define the power on/off order.
> >
> > >>> + #power-domain-cells = <0x0>;
> > >>> + };
> > >>> + };
> >
>
> I agree it should be arranged in as parent child order to control subset or control
> order. Will incorporate those changes in next version.
>
> > Best regards
> > --
> > Marek Szyprowski, PhD
> > Samsung R&D Institute Poland
As suggested, I tried out parent, child approach. However what I found is Genpd core takes care of parent child dependencies for power on off routines only. In our case, We need them in attach-detach routines too. In that case, we need to handle dependencies manually for those routines. Please suggest better approach, if any.
Thanks,
Jolly Shah
^ permalink raw reply
* Re: [PATCH 1/3] ARM: dra762: hwmod: Add MCAN support
From: Tony Lindgren @ 2018-05-17 21:09 UTC (permalink / raw)
To: Faiz Abbas
Cc: linux-kernel, linux-omap, devicetree, bcousson, robh+dt,
mark.rutland, paul, lokeshvutla, linux
In-Reply-To: <1523181542-3770-2-git-send-email-faiz_abbas@ti.com>
* Faiz Abbas <faiz_abbas@ti.com> [180408 09:59]:
> From: Lokesh Vutla <lokeshvutla@ti.com>
>
> Add MCAN hwmod data and register it for dra762 silicons.
>
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> ---
> arch/arm/mach-omap2/omap_hwmod_7xx_data.c | 32 +++++++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
>
> diff --git a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
> index 62352d1..a2cd7f8 100644
> --- a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
> +++ b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
> @@ -1356,6 +1356,29 @@ static struct omap_hwmod dra7xx_mailbox13_hwmod = {
> };
>
> /*
> + * 'mcan' class
> + *
> + */
> +static struct omap_hwmod_class dra76x_mcan_hwmod_class = {
> + .name = "mcan",
> +};
> +
> +/* mcan */
> +static struct omap_hwmod dra76x_mcan_hwmod = {
> + .name = "mcan",
> + .class = &dra76x_mcan_hwmod_class,
> + .clkdm_name = "wkupaon_clkdm",
> + .main_clk = "mcan_clk",
> + .prcm = {
> + .omap4 = {
> + .clkctrl_offs = DRA7XX_CM_WKUPAON_ADC_CLKCTRL_OFFSET,
> + .context_offs = DRA7XX_RM_WKUPAON_ADC_CONTEXT_OFFSET,
> + .modulemode = MODULEMODE_SWCTRL,
> + },
> + },
> +};
So based on the ti-sysc related dts comments, this patch should
work except you should be able to just leave out the clocks here
with the dts changes.
> +/*
> * 'mcspi' class
> *
> */
> @@ -3818,6 +3841,14 @@ static struct omap_hwmod_ocp_if dra7xx_l4_per2__epwmss2 = {
> .user = OCP_USER_MPU,
> };
>
> +/* l3_main_1 -> mcan */
> +static struct omap_hwmod_ocp_if dra76x_l3_main_1__mcan = {
> + .master = &dra7xx_l3_main_1_hwmod,
> + .slave = &dra76x_mcan_hwmod,
> + .clk = "l3_iclk_div",
> + .user = OCP_USER_MPU | OCP_USER_SDMA,
> +};
> +
> static struct omap_hwmod_ocp_if *dra7xx_hwmod_ocp_ifs[] __initdata = {
> &dra7xx_l3_main_1__dmm,
> &dra7xx_l3_main_2__l3_instr,
> @@ -3958,6 +3989,7 @@ static struct omap_hwmod_ocp_if *dra7xx_gp_hwmod_ocp_ifs[] __initdata = {
> /* SoC variant specific hwmod links */
> static struct omap_hwmod_ocp_if *dra76x_hwmod_ocp_ifs[] __initdata = {
> &dra7xx_l4_per3__usb_otg_ss4,
> + &dra76x_l3_main_1__mcan,
> NULL,
> };
So the omap_hwmod_class, omap_hwmod_ocp_if and entry on the
dra76x_hwmod_ocp_ifs list are still needed with ti-sysc for a
while. Eventually that data will just come from the dts interconnect
hierarchy. For struct omap_hwmod_class_sysconfig, ti-sysc will
allocate and it based on the dts data.
Regards,
Tony
^ permalink raw reply
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