* Re: [PATCH V6 14/21] clk: tegra210: Add suspend and resume support
From: Stephen Boyd @ 2019-08-07 21:22 UTC (permalink / raw)
To: Dmitry Osipenko, Sowjanya Komatineni, jason, jonathanh,
linus.walleij, marc.zyngier, mark.rutland, stefan, tglx,
thierry.reding
Cc: pdeschrijver, pgaikwad, linux-clk, linux-gpio, jckuo, josephl,
talho, linux-tegra, linux-kernel, mperttunen, spatra, robh+dt,
devicetree
In-Reply-To: <641727e6-4796-f982-3b58-4c8d666de1a2@nvidia.com>
Quoting Sowjanya Komatineni (2019-08-02 13:39:57)
>
> On 8/2/19 10:51 AM, Stephen Boyd wrote:
> > And also add a comment to this location in the code because it's
> > non-obvious that we can't use iopoll here.
> >
> Actually added comment during function usage instead of during include
> as iopoll.h is removed.
>
> Will add additional comment in include section as well highlighting
> reason for removal of iopoll.h
>
No I wasn't saying to add a comment to the include section, just add a
comment in the place where you would have called iopoll but don't. Sorry
that it wasn't clear.
^ permalink raw reply
* Re: [PATCH v1 1/2] clk: qcom: Add DT bindings for SC7180 gcc clock controller
From: Stephen Boyd @ 2019-08-07 22:14 UTC (permalink / raw)
Cc: David Brown, Rajendra Nayak, linux-arm-msm, linux-soc, linux-clk,
linux-kernel, devicetree, robh, Taniya Das
In-Reply-To: <20190807181301.15326-2-tdas@codeaurora.org>
Quoting Taniya Das (2019-08-07 11:13:00)
> diff --git a/Documentation/devicetree/bindings/clock/qcom,gcc.txt b/Documentation/devicetree/bindings/clock/qcom,gcc.txt
> index 8661c3cd3ccf..18d95467cb36 100644
> --- a/Documentation/devicetree/bindings/clock/qcom,gcc.txt
> +++ b/Documentation/devicetree/bindings/clock/qcom,gcc.txt
> @@ -23,6 +23,7 @@ Required properties :
> "qcom,gcc-sdm630"
> "qcom,gcc-sdm660"
> "qcom,gcc-sdm845"
> + "qcom,gcc-sc7180"
>
> - reg : shall contain base register location and length
> - #clock-cells : shall contain 1
Can you please list the clk inputs that you're expecting now? Similar to
what Vinod did for the other most recent GCC driver. We should probably
convert this binding to YAML.
^ permalink raw reply
* Re: [PATCH v1 2/2] clk: qcom: Add Global Clock controller (GCC) driver for SC7180
From: Stephen Boyd @ 2019-08-07 22:24 UTC (permalink / raw)
Cc: David Brown, Rajendra Nayak, linux-arm-msm, linux-soc, linux-clk,
linux-kernel, devicetree, robh, Taniya Das
In-Reply-To: <20190807181301.15326-3-tdas@codeaurora.org>
Quoting Taniya Das (2019-08-07 11:13:01)
> Add support for the global clock controller found on SC7180
> based devices. This should allow most non-multimedia device
> drivers to probe and control their clocks.
>
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
Overall looks pretty good to me. Some minor nitpicks below.
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index e1ff83cc361e..9e634cf27464 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -323,4 +323,13 @@ config KRAITCC
> Support for the Krait CPU clocks on Qualcomm devices.
> Say Y if you want to support CPU frequency scaling.
>
> +config SC_GCC_7180
Can this be sorted alphabetically in this file? It's getting out of hand
when they all get added to the end.
> + tristate "SC7180 Global Clock Controller"
> + select QCOM_GDSC
> + depends on COMMON_CLK_QCOM
> + help
> + Support for the global clock controller on SC7180 devices.
> + Say Y if you want to use peripheral devices such as UART, SPI,
> + I2C, USB, UFS, SDCC, etc.
> +
> endif
> diff --git a/drivers/clk/qcom/gcc-sc7180.c b/drivers/clk/qcom/gcc-sc7180.c
> new file mode 100644
> index 000000000000..004b76e69402
> --- /dev/null
> +++ b/drivers/clk/qcom/gcc-sc7180.c
> @@ -0,0 +1,2496 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +
> +#include <dt-bindings/clock/qcom,gcc-sc7180.h>
> +
> +#include "clk-alpha-pll.h"
> +#include "clk-branch.h"
> +#include "clk-rcg.h"
> +#include "clk-regmap.h"
> +#include "common.h"
> +#include "gdsc.h"
> +#include "reset.h"
> +
> +#define GCC_MMSS_MISC 0x09ffc
> +#define GCC_NPU_MISC 0x4d110
> +#define GCC_GPU_MISC 0x71028
> +
> +enum {
> + P_BI_TCXO,
> + P_CORE_BI_PLL_TEST_SE,
> + P_GPLL0_OUT_EVEN,
> + P_GPLL0_OUT_MAIN,
> + P_GPLL1_OUT_MAIN,
> + P_GPLL4_OUT_MAIN,
> + P_GPLL6_OUT_MAIN,
> + P_GPLL7_OUT_MAIN,
> + P_SLEEP_CLK,
> +};
> +
> +static struct clk_alpha_pll gpll0;
> +static struct clk_alpha_pll gpll1;
> +static struct clk_alpha_pll gpll4;
> +static struct clk_alpha_pll gpll6;
> +static struct clk_alpha_pll gpll7;
> +static struct clk_alpha_pll_postdiv gpll0_out_even;
Can't we create these PLLs before the parent maps? I don't see why we
need to forward declare the structs.
> +
> +static const struct parent_map gcc_parent_map_0[] = {
> + { P_BI_TCXO, 0 },
> + { P_GPLL0_OUT_MAIN, 1 },
> + { P_GPLL0_OUT_EVEN, 6 },
> + { P_CORE_BI_PLL_TEST_SE, 7 },
> +};
> +
> +static const struct clk_parent_data gcc_parent_data_0[] = {
> + { .fw_name = "bi_tcxo", .name = "bi_tcxo" },
> + { .hw = &gpll0.clkr.hw },
[...]
> +static struct clk_branch gcc_camera_ahb_clk = {
> + .halt_reg = 0xb008,
> + .halt_check = BRANCH_HALT,
> + .hwcg_reg = 0xb008,
> + .hwcg_bit = 1,
> + .clkr = {
> + .enable_reg = 0xb008,
> + .enable_mask = BIT(0),
> + .hw.init = &(struct clk_init_data){
> + .name = "gcc_camera_ahb_clk",
> + .flags = CLK_IS_CRITICAL,
Please add a comment for CLK_IS_CRITICAL usage explaining why clks must
be kept on forever.
> + .ops = &clk_branch2_ops,
> + },
> + },
> +};
> +
> +static struct clk_branch gcc_camera_hf_axi_clk = {
> + .halt_reg = 0xb020,
> + .halt_check = BRANCH_HALT,
> + .clkr = {
> + .enable_reg = 0xb020,
> + .enable_mask = BIT(0),
> + .hw.init = &(struct clk_init_data){
[...]
> +
> +static const struct qcom_reset_map gcc_sc7180_resets[] = {
> + [GCC_QUSB2PHY_PRIM_BCR] = { 0x26000 },
> + [GCC_QUSB2PHY_SEC_BCR] = { 0x26004 },
> + [GCC_UFS_PHY_BCR] = { 0x77000 },
> + [GCC_USB30_PRIM_BCR] = { 0xf000 },
> + [GCC_USB3_PHY_PRIM_BCR] = { 0x50000 },
> + [GCC_USB3PHY_PHY_PRIM_BCR] = { 0x50004 },
> + [GCC_USB3_PHY_SEC_BCR] = { 0x5000c },
> + [GCC_USB3_DP_PHY_PRIM_BCR] = { 0x50008 },
> + [GCC_USB3PHY_PHY_SEC_BCR] = { 0x50010 },
> + [GCC_USB3_DP_PHY_SEC_BCR] = { 0x50014 },
> + [GCC_USB_PHY_CFG_AHB2PHY_BCR] = { 0x6a000 },
> +};
> +
> +static struct clk_rcg_dfs_data gcc_dfs_clocks[] = {
> + DEFINE_RCG_DFS(gcc_qupv3_wrap0_s0_clk),
I was happy if you wanted to include the _src changing patch from
earlier. Can you throw it into this series?
> + DEFINE_RCG_DFS(gcc_qupv3_wrap0_s1_clk),
> + DEFINE_RCG_DFS(gcc_qupv3_wrap0_s2_clk),
> + DEFINE_RCG_DFS(gcc_qupv3_wrap0_s3_clk),
> + DEFINE_RCG_DFS(gcc_qupv3_wrap0_s4_clk),
> + DEFINE_RCG_DFS(gcc_qupv3_wrap0_s5_clk),
> + DEFINE_RCG_DFS(gcc_qupv3_wrap1_s0_clk),
> + DEFINE_RCG_DFS(gcc_qupv3_wrap1_s1_clk),
> + DEFINE_RCG_DFS(gcc_qupv3_wrap1_s2_clk),
> + DEFINE_RCG_DFS(gcc_qupv3_wrap1_s3_clk),
> + DEFINE_RCG_DFS(gcc_qupv3_wrap1_s4_clk),
> + DEFINE_RCG_DFS(gcc_qupv3_wrap1_s5_clk),
> +};
> +
> +static const struct regmap_config gcc_sc7180_regmap_config = {
> + .reg_bits = 32,
> + .reg_stride = 4,
> + .val_bits = 32,
> + .max_register = 0x18208c,
> + .fast_io = true,
> +};
> +
> +static const struct qcom_cc_desc gcc_sc7180_desc = {
> + .config = &gcc_sc7180_regmap_config,
> + .clk_hws = gcc_sc7180_hws,
> + .num_clk_hws = ARRAY_SIZE(gcc_sc7180_hws),
> + .clks = gcc_sc7180_clocks,
> + .num_clks = ARRAY_SIZE(gcc_sc7180_clocks),
> + .resets = gcc_sc7180_resets,
> + .num_resets = ARRAY_SIZE(gcc_sc7180_resets),
> + .gdscs = gcc_sc7180_gdscs,
> + .num_gdscs = ARRAY_SIZE(gcc_sc7180_gdscs),
> +};
> +
> +static const struct of_device_id gcc_sc7180_match_table[] = {
> + { .compatible = "qcom,gcc-sc7180" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, gcc_sc7180_match_table);
> +
> +static int gcc_sc7180_probe(struct platform_device *pdev)
> +{
> + struct regmap *regmap;
> + int ret;
> +
> + regmap = qcom_cc_map(pdev, &gcc_sc7180_desc);
> + if (IS_ERR(regmap)) {
> + pr_err("Failed to map the gcc registers\n");
I don't think we need this error message. Please remove it.
> + return PTR_ERR(regmap);
> + }
> +
> + /*
> + * Disable the GPLL0 active input to MM blocks, NPU
> + * and GPU via MISC registers.
> + */
> + regmap_update_bits(regmap, GCC_MMSS_MISC, 0x3, 0x3);
> + regmap_update_bits(regmap, GCC_NPU_MISC, 0x3, 0x3);
> + regmap_update_bits(regmap, GCC_GPU_MISC, 0x3, 0x3);
> +
> + /* DFS clock registration */
This comment is pretty useless, please remove it.
> + ret = qcom_cc_register_rcg_dfs(regmap, gcc_dfs_clocks,
> + ARRAY_SIZE(gcc_dfs_clocks));
> + if (ret)
> + return ret;
> +
> + return qcom_cc_really_probe(pdev, &gcc_sc7180_desc, regmap);
> +}
^ permalink raw reply
* [PATCH v5 0/3] Introduce Bandwidth OPPs for interconnects
From: Saravana Kannan @ 2019-08-07 22:31 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Viresh Kumar, Nishanth Menon,
Stephen Boyd, Rafael J. Wysocki
Cc: Saravana Kannan, Georgi Djakov, vincent.guittot, seansw,
daidavid1, adharmap, Rajendra Nayak, sibis, bjorn.andersson,
evgreen, kernel-team, linux-pm, devicetree, linux-kernel
Interconnects and interconnect paths quantify their performance levels in
terms of bandwidth and not in terms of frequency. So similar to how we have
frequency based OPP tables in DT and in the OPP framework, we need
bandwidth OPP table support in DT and in the OPP framework.
So with the DT bindings added in this patch series, the DT for a GPU
that does bandwidth voting from GPU to Cache and GPU to DDR would look
something like this:
gpu_cache_opp_table: gpu_cache_opp_table {
compatible = "operating-points-v2";
gpu_cache_3000: opp-3000 {
opp-peak-KBps = <3000000>;
opp-avg-KBps = <1000000>;
};
gpu_cache_6000: opp-6000 {
opp-peak-KBps = <6000000>;
opp-avg-KBps = <2000000>;
};
gpu_cache_9000: opp-9000 {
opp-peak-KBps = <9000000>;
opp-avg-KBps = <9000000>;
};
};
gpu_ddr_opp_table: gpu_ddr_opp_table {
compatible = "operating-points-v2";
gpu_ddr_1525: opp-1525 {
opp-peak-KBps = <1525000>;
opp-avg-KBps = <452000>;
};
gpu_ddr_3051: opp-3051 {
opp-peak-KBps = <3051000>;
opp-avg-KBps = <915000>;
};
gpu_ddr_7500: opp-7500 {
opp-peak-KBps = <7500000>;
opp-avg-KBps = <3000000>;
};
};
gpu_opp_table: gpu_opp_table {
compatible = "operating-points-v2";
opp-shared;
opp-200000000 {
opp-hz = /bits/ 64 <200000000>;
};
opp-400000000 {
opp-hz = /bits/ 64 <400000000>;
};
};
gpu@7864000 {
...
operating-points-v2 = <&gpu_opp_table>, <&gpu_cache_opp_table>, <&gpu_ddr_opp_table>;
...
};
v1 -> v3:
- Lots of patch additions that were later dropped
v3 -> v4:
- Fixed typo bugs pointed out by Sibi.
- Fixed bug that incorrectly reset rate to 0 all the time
- Added units documentation
- Dropped interconnect-opp-table property and related changes
v4->v5:
- Replaced KBps with kBps
- Minor documentation fix
Cheers,
Saravana
Saravana Kannan (3):
dt-bindings: opp: Introduce opp-peak-kBps and opp-avg-kBps bindings
OPP: Add support for bandwidth OPP tables
OPP: Add helper function for bandwidth OPP tables
Documentation/devicetree/bindings/opp/opp.txt | 15 ++++--
.../devicetree/bindings/property-units.txt | 4 ++
drivers/opp/core.c | 51 +++++++++++++++++++
drivers/opp/of.c | 41 +++++++++++----
drivers/opp/opp.h | 4 +-
include/linux/pm_opp.h | 19 +++++++
6 files changed, 121 insertions(+), 13 deletions(-)
--
2.23.0.rc1.153.gdeed80330f-goog
^ permalink raw reply
* [PATCH v5 1/3] dt-bindings: opp: Introduce opp-peak-kBps and opp-avg-kBps bindings
From: Saravana Kannan @ 2019-08-07 22:31 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Viresh Kumar, Nishanth Menon,
Stephen Boyd, Rafael J. Wysocki
Cc: Saravana Kannan, Georgi Djakov, vincent.guittot, seansw,
daidavid1, adharmap, Rajendra Nayak, sibis, bjorn.andersson,
evgreen, kernel-team, linux-pm, devicetree, linux-kernel
In-Reply-To: <20190807223111.230846-1-saravanak@google.com>
Interconnects often quantify their performance points in terms of
bandwidth. So, add opp-peak-kBps (required) and opp-avg-kBps (optional) to
allow specifying Bandwidth OPP tables in DT.
opp-peak-kBps is a required property that replaces opp-hz for Bandwidth OPP
tables.
opp-avg-kBps is an optional property that can be used in Bandwidth OPP
tables.
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
Documentation/devicetree/bindings/opp/opp.txt | 15 ++++++++++++---
.../devicetree/bindings/property-units.txt | 4 ++++
2 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
index 68592271461f..dbad8eb6c746 100644
--- a/Documentation/devicetree/bindings/opp/opp.txt
+++ b/Documentation/devicetree/bindings/opp/opp.txt
@@ -83,9 +83,14 @@ properties.
Required properties:
- opp-hz: Frequency in Hz, expressed as a 64-bit big-endian integer. This is a
- required property for all device nodes but devices like power domains. The
- power domain nodes must have another (implementation dependent) property which
- uniquely identifies the OPP nodes.
+ required property for all device nodes except for devices like power domains
+ or bandwidth opp tables. The power domain nodes must have another
+ (implementation dependent) property which uniquely identifies the OPP nodes.
+ The interconnect opps are required to have the opp-peak-kBps property.
+
+- opp-peak-kBps: Peak bandwidth in kilobytes per second, expressed as a 32-bit
+ big-endian integer. This is a required property for all devices that don't
+ have opp-hz. For example, bandwidth OPP tables for interconnect paths.
Optional properties:
- opp-microvolt: voltage in micro Volts.
@@ -132,6 +137,10 @@ Optional properties:
- opp-level: A value representing the performance level of the device,
expressed as a 32-bit integer.
+- opp-avg-kBps: Average bandwidth in kilobytes per second, expressed as a
+ 32-bit big-endian integer. This property is only meaningful in OPP tables
+ where opp-peak-kBps is present.
+
- clock-latency-ns: Specifies the maximum possible transition latency (in
nanoseconds) for switching to this OPP from any other OPP.
diff --git a/Documentation/devicetree/bindings/property-units.txt b/Documentation/devicetree/bindings/property-units.txt
index e9b8360b3288..c80a110c1e26 100644
--- a/Documentation/devicetree/bindings/property-units.txt
+++ b/Documentation/devicetree/bindings/property-units.txt
@@ -41,3 +41,7 @@ Temperature
Pressure
----------------------------------------
-kpascal : kilopascal
+
+Throughput
+----------------------------------------
+-kBps : kilobytes per second
--
2.23.0.rc1.153.gdeed80330f-goog
^ permalink raw reply related
* [PATCH v5 2/3] OPP: Add support for bandwidth OPP tables
From: Saravana Kannan @ 2019-08-07 22:31 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Viresh Kumar, Nishanth Menon,
Stephen Boyd, Rafael J. Wysocki
Cc: Saravana Kannan, Georgi Djakov, vincent.guittot, seansw,
daidavid1, adharmap, Rajendra Nayak, sibis, bjorn.andersson,
evgreen, kernel-team, linux-pm, devicetree, linux-kernel
In-Reply-To: <20190807223111.230846-1-saravanak@google.com>
Not all devices quantify their performance points in terms of frequency.
Devices like interconnects quantify their performance points in terms of
bandwidth. We need a way to represent these bandwidth levels in OPP. So,
add support for parsing bandwidth OPPs from DT.
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
drivers/opp/of.c | 41 ++++++++++++++++++++++++++++++++---------
drivers/opp/opp.h | 4 +++-
2 files changed, 35 insertions(+), 10 deletions(-)
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 1813f5ad5fa2..e1750033fef9 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -523,6 +523,35 @@ void dev_pm_opp_of_remove_table(struct device *dev)
}
EXPORT_SYMBOL_GPL(dev_pm_opp_of_remove_table);
+static int _read_opp_key(struct dev_pm_opp *new_opp, struct device_node *np)
+{
+ int ret;
+ u64 rate;
+ u32 bw;
+
+ ret = of_property_read_u64(np, "opp-hz", &rate);
+ if (!ret) {
+ /*
+ * Rate is defined as an unsigned long in clk API, and so
+ * casting explicitly to its type. Must be fixed once rate is 64
+ * bit guaranteed in clk API.
+ */
+ new_opp->rate = (unsigned long)rate;
+ return 0;
+ }
+
+ ret = of_property_read_u32(np, "opp-peak-kBps", &bw);
+ if (ret)
+ return ret;
+ new_opp->rate = (unsigned long) bw;
+
+ ret = of_property_read_u32(np, "opp-avg-kBps", &bw);
+ if (!ret)
+ new_opp->avg_bw = (unsigned long) bw;
+
+ return 0;
+}
+
/**
* _opp_add_static_v2() - Allocate static OPPs (As per 'v2' DT bindings)
* @opp_table: OPP table
@@ -560,22 +589,16 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
if (!new_opp)
return ERR_PTR(-ENOMEM);
- ret = of_property_read_u64(np, "opp-hz", &rate);
+ ret = _read_opp_key(new_opp, np);
if (ret < 0) {
/* "opp-hz" is optional for devices like power domains. */
if (!opp_table->is_genpd) {
- dev_err(dev, "%s: opp-hz not found\n", __func__);
+ dev_err(dev, "%s: opp-hz or opp-peak-kBps not found\n",
+ __func__);
goto free_opp;
}
rate_not_available = true;
- } else {
- /*
- * Rate is defined as an unsigned long in clk API, and so
- * casting explicitly to its type. Must be fixed once rate is 64
- * bit guaranteed in clk API.
- */
- new_opp->rate = (unsigned long)rate;
}
of_property_read_u32(np, "opp-level", &new_opp->level);
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 01a500e2c40a..6bb238af9cac 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -56,7 +56,8 @@ extern struct list_head opp_tables;
* @turbo: true if turbo (boost) OPP
* @suspend: true if suspend OPP
* @pstate: Device's power domain's performance state.
- * @rate: Frequency in hertz
+ * @rate: Frequency in hertz OR Peak bandwidth in kilobytes per second
+ * @avg_bw: Average bandwidth in kilobytes per second
* @level: Performance level
* @supplies: Power supplies voltage/current values
* @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's
@@ -78,6 +79,7 @@ struct dev_pm_opp {
bool suspend;
unsigned int pstate;
unsigned long rate;
+ unsigned long avg_bw;
unsigned int level;
struct dev_pm_opp_supply *supplies;
--
2.23.0.rc1.153.gdeed80330f-goog
^ permalink raw reply related
* [PATCH v5 3/3] OPP: Add helper function for bandwidth OPP tables
From: Saravana Kannan @ 2019-08-07 22:31 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Viresh Kumar, Nishanth Menon,
Stephen Boyd, Rafael J. Wysocki
Cc: Saravana Kannan, Georgi Djakov, vincent.guittot, seansw,
daidavid1, adharmap, Rajendra Nayak, sibis, bjorn.andersson,
evgreen, kernel-team, linux-pm, devicetree, linux-kernel
In-Reply-To: <20190807223111.230846-1-saravanak@google.com>
The frequency OPP tables have helper functions to search for entries in the
table based on frequency and get the frequency values for a given (or
suspend) OPP entry.
Add similar helper functions for bandwidth OPP tables to search for entries
in the table based on peak bandwidth and to get the peak and average
bandwidth for a given (or suspend) OPP entry.
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
drivers/opp/core.c | 51 ++++++++++++++++++++++++++++++++++++++++++
include/linux/pm_opp.h | 19 ++++++++++++++++
2 files changed, 70 insertions(+)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 3b7ffd0234e9..22dcf22f908f 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -127,6 +127,29 @@ unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp)
}
EXPORT_SYMBOL_GPL(dev_pm_opp_get_freq);
+/**
+ * dev_pm_opp_get_bw() - Gets the bandwidth corresponding to an available opp
+ * @opp: opp for which frequency has to be returned for
+ * @avg_bw: Pointer where the corresponding average bandwidth is stored.
+ * Can be NULL.
+ *
+ * Return: Peak bandwidth in kBps corresponding to the opp, else
+ * return 0
+ */
+unsigned long dev_pm_opp_get_bw(struct dev_pm_opp *opp, unsigned long *avg_bw)
+{
+ if (IS_ERR_OR_NULL(opp) || !opp->available) {
+ pr_err("%s: Invalid parameters\n", __func__);
+ return 0;
+ }
+
+ if (avg_bw)
+ *avg_bw = opp->avg_bw;
+
+ return opp->rate;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_get_bw);
+
/**
* dev_pm_opp_get_level() - Gets the level corresponding to an available opp
* @opp: opp for which level value has to be returned for
@@ -299,6 +322,34 @@ unsigned long dev_pm_opp_get_suspend_opp_freq(struct device *dev)
}
EXPORT_SYMBOL_GPL(dev_pm_opp_get_suspend_opp_freq);
+/**
+ * dev_pm_opp_get_suspend_opp_bw() - Get peak bandwidth of suspend opp in kBps
+ * @dev: device for which we do this operation
+ * @avg_bw: Pointer where the corresponding average bandwidth is stored.
+ * Can be NULL.
+ *
+ * Return: This function returns the peak bandwidth of the OPP marked as
+ * suspend_opp if one is available, else returns 0;
+ */
+unsigned long dev_pm_opp_get_suspend_opp_bw(struct device *dev,
+ unsigned long *avg_bw)
+{
+ struct opp_table *opp_table;
+ unsigned long peak_bw = 0;
+
+ opp_table = _find_opp_table(dev);
+ if (IS_ERR(opp_table))
+ return 0;
+
+ if (opp_table->suspend_opp && opp_table->suspend_opp->available)
+ peak_bw = dev_pm_opp_get_bw(opp_table->suspend_opp, avg_bw);
+
+ dev_pm_opp_put_opp_table(opp_table);
+
+ return peak_bw;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_get_suspend_opp_bw);
+
int _get_opp_count(struct opp_table *opp_table)
{
struct dev_pm_opp *opp;
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index b8197ab014f2..f4e900f36414 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -82,6 +82,7 @@ void dev_pm_opp_put_opp_table(struct opp_table *opp_table);
unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp);
unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp);
+unsigned long dev_pm_opp_get_bw(struct dev_pm_opp *opp, unsigned long *avg_bw);
unsigned int dev_pm_opp_get_level(struct dev_pm_opp *opp);
@@ -92,6 +93,8 @@ unsigned long dev_pm_opp_get_max_clock_latency(struct device *dev);
unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev);
unsigned long dev_pm_opp_get_max_transition_latency(struct device *dev);
unsigned long dev_pm_opp_get_suspend_opp_freq(struct device *dev);
+unsigned long dev_pm_opp_get_suspend_opp_bw(struct device *dev,
+ unsigned long *avg_bw);
struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
unsigned long freq,
@@ -160,6 +163,11 @@ static inline unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp)
{
return 0;
}
+static inline unsigned long dev_pm_opp_get_bw(struct dev_pm_opp *opp,
+ unsigned long *avg_bw)
+{
+ return 0;
+}
static inline unsigned int dev_pm_opp_get_level(struct dev_pm_opp *opp)
{
@@ -196,6 +204,12 @@ static inline unsigned long dev_pm_opp_get_suspend_opp_freq(struct device *dev)
return 0;
}
+static inline unsigned long dev_pm_opp_get_suspend_opp_bw(struct device *dev,
+ unsigned long *avg_bw)
+{
+ return 0;
+}
+
static inline struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
unsigned long freq, bool available)
{
@@ -337,6 +351,11 @@ static inline void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask
#endif /* CONFIG_PM_OPP */
+#define dev_pm_opp_find_peak_bw_exact dev_pm_opp_find_freq_exact
+#define dev_pm_opp_find_peak_bw_floor dev_pm_opp_find_freq_floor
+#define dev_pm_opp_find_peak_bw_ceil_by_volt dev_pm_opp_find_freq_ceil_by_volt
+#define dev_pm_opp_find_peak_bw_ceil dev_pm_opp_find_freq_ceil
+
#if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)
int dev_pm_opp_of_add_table(struct device *dev);
int dev_pm_opp_of_add_table_indexed(struct device *dev, int index);
--
2.23.0.rc1.153.gdeed80330f-goog
^ permalink raw reply related
* Re: [RFCv3 1/3] dt-bindings: interconnect: Add bindings for imx
From: Leonard Crestez @ 2019-08-07 22:35 UTC (permalink / raw)
To: Georgi Djakov, Rob Herring
Cc: Mark Rutland, Artur Świgoń, Jacky Bai, Saravana Kannan,
Anson Huang, Stephen Boyd, Viresh Kumar, Michael Turquette,
linux-pm@vger.kernel.org, Krzysztof Kozlowski, Chanwoo Choi,
Kyungmin Park, MyungJoo Ham, Alexandre Bailon,
kernel@pengutronix.de, Fabio Estevam,
linux-arm-kernel@lists.infradead.org, Shawn Guo, Aisheng Dong,
devicetree@vger.kernel.org
In-Reply-To: <123536fc-c3ce-5bfe-fbd6-20cde0c13cc0@linaro.org>
On 8/7/2019 6:16 PM, Georgi Djakov wrote:
> Please put some commit text.
Will fix
>> +properties:
>> + compatible:
>> + contains:
>> + enum:
>> + - fsl,imx8mm-interconnect
>
> Maybe fsl,imx8mm-busfreq? I thought it's called busfreq in downstream, but it's
> up to you.
In the vendor tree busfreq is effectively its own subsystem with its own
API calls used by imx drivers. As part of replacing this with standard
devfreq + interconnect in upstream the old name should go away.
--
Regards,
Leonard
^ permalink raw reply
* Re: [PATCH] dt-bindings: clock: imx8mn: Fix tab indentation for yaml file
From: Stephen Boyd @ 2019-08-07 23:59 UTC (permalink / raw)
To: Rob Herring
Cc: Anson Huang, devicetree, Fabio Estevam, Sascha Hauer,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-clk, linux-kernel@vger.kernel.org, Mark Rutland,
Michael Turquette, Sascha Hauer, Shawn Guo, NXP Linux Team
In-Reply-To: <CAL_JsqKZHG-y_cKitU0=EksgyVU-YLOi1gAcFXx4ve21CMki1g@mail.gmail.com>
Quoting Rob Herring (2019-07-25 14:37:24)
> On Thu, Jul 25, 2019 at 3:06 PM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Anson.Huang@nxp.com (2019-07-24 19:05:51)
> > > From: Anson Huang <Anson.Huang@nxp.com>
> > >
> > > YAML file can NOT contain tab as indentation, fix it.
> > >
> >
> > Would be nice if checkpatch could check for this.
>
> Would be nice if folks just ran 'make dt_binding_check'. :) It
> wouldn't be hard to add a tab check to checkpatch, but that's just one
> potential problem.
>
Cool, thanks for the pointer. Seems I forgot :)
Here's a patch to improve the documentation and make help to answer
questions I had about how to work this into my workflow.
diff --git a/Documentation/devicetree/writing-schema.md b/Documentation/devicetree/writing-schema.md
index dc032db36262..17ad67887fde 100644
--- a/Documentation/devicetree/writing-schema.md
+++ b/Documentation/devicetree/writing-schema.md
@@ -120,6 +120,7 @@ This will first run the `dt_binding_check` which generates the processed schema.
It is also possible to run checks with a single schema file by setting the
'DT_SCHEMA_FILES' variable to a specific schema file.
+`make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/trivial-devices.yaml`
`make dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/trivial-devices.yaml`
diff --git a/Makefile b/Makefile
index 9be5834073f8..96bb28aa1c46 100644
--- a/Makefile
+++ b/Makefile
@@ -1503,8 +1503,10 @@ help:
@echo ''
@$(if $(dtstree), \
echo 'Devicetree:'; \
- echo '* dtbs - Build device tree blobs for enabled boards'; \
- echo ' dtbs_install - Install dtbs to $(INSTALL_DTBS_PATH)'; \
+ echo '* dtbs - Build device tree blobs for enabled boards'; \
+ echo ' dtbs_install - Install dtbs to $(INSTALL_DTBS_PATH)'; \
+ echo ' dt_binding_check - Validate device tree binding documents'; \
+ echo ' dtbs_check - Validate device tree source files'; \
echo '')
@echo 'Userspace tools targets:'
^ permalink raw reply related
* [PATCH v3] dt-bindings: rcar-{csi2,vin}: Rename bindings documentation files
From: Niklas Söderlund @ 2019-08-08 1:03 UTC (permalink / raw)
To: Geert Uytterhoeven, Rob Herring, devicetree, linux-media
Cc: linux-renesas-soc, Niklas Söderlund, Geert Uytterhoeven,
Ulrich Hecht, Simon Horman
Renesas media binding documentation files uses a naming schema of
'renesas,<module>.txt'. Rename VIN and CSI-2 files to match this
pattern.
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
---
.../media/{renesas,rcar-csi2.txt => renesas,csi2.txt} | 0
.../bindings/media/{rcar_vin.txt => renesas,vin.txt} | 0
MAINTAINERS | 4 ++--
3 files changed, 2 insertions(+), 2 deletions(-)
rename Documentation/devicetree/bindings/media/{renesas,rcar-csi2.txt => renesas,csi2.txt} (100%)
rename Documentation/devicetree/bindings/media/{rcar_vin.txt => renesas,vin.txt} (100%)
---
Hi Geert,
Would you be willing to take this patch in your renesas tree? There
seems to be a lack of interest in it :-(
// Niklas
diff --git a/Documentation/devicetree/bindings/media/renesas,rcar-csi2.txt b/Documentation/devicetree/bindings/media/renesas,csi2.txt
similarity index 100%
rename from Documentation/devicetree/bindings/media/renesas,rcar-csi2.txt
rename to Documentation/devicetree/bindings/media/renesas,csi2.txt
diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt b/Documentation/devicetree/bindings/media/renesas,vin.txt
similarity index 100%
rename from Documentation/devicetree/bindings/media/rcar_vin.txt
rename to Documentation/devicetree/bindings/media/renesas,vin.txt
diff --git a/MAINTAINERS b/MAINTAINERS
index 3012e33e59465f4c..60bd9b419ee1f635 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9801,8 +9801,8 @@ L: linux-media@vger.kernel.org
L: linux-renesas-soc@vger.kernel.org
T: git git://linuxtv.org/media_tree.git
S: Supported
-F: Documentation/devicetree/bindings/media/renesas,rcar-csi2.txt
-F: Documentation/devicetree/bindings/media/rcar_vin.txt
+F: Documentation/devicetree/bindings/media/renesas,csi2.txt
+F: Documentation/devicetree/bindings/media/renesas,vin.txt
F: drivers/media/platform/rcar-vin/
MEDIA DRIVERS FOR RENESAS - VSP1
--
2.22.0
^ permalink raw reply related
* Re: [PATCH v5 2/4] devfreq: exynos-bus: convert to use dev_pm_opp_set_rate()
From: Chanwoo Choi @ 2019-08-08 1:21 UTC (permalink / raw)
To: k.konieczny
Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, Krzysztof Kozlowski,
Kukjin Kim, Kyungmin Park, Mark Rutland, MyungJoo Ham,
Nishanth Menon, Rob Herring, Stephen Boyd, Viresh Kumar,
devicetree, linux-arm-kernel, linux-kernel, linux-pm,
linux-samsung-soc
In-Reply-To: <20190807133838.14678-3-k.konieczny@partner.samsung.com>
Hi Kamil,
On 19. 8. 7. 오후 10:38, k.konieczny@partner.samsung.com wrote:
> Reuse opp core code for setting bus clock and voltage. As a side
> effect this allow usage of coupled regulators feature (required
> for boards using Exynos5422/5800 SoCs) because dev_pm_opp_set_rate()
> uses regulator_set_voltage_triplet() for setting regulator voltage
> while the old code used regulator_set_voltage_tol() with fixed
> tolerance. This patch also removes no longer needed parsing of DT
> property "exynos,voltage-tolerance" (no Exynos devfreq DT node uses
> it). After applying changes both functions exynos_bus_passive_target()
> and exynos_bus_target() have the same code, so remove
> exynos_bus_passive_target(). In exynos_bus_probe() replace it with
> exynos_bus_target.
>
> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
> ---
> Changes:
> v5:
> - squashed last patch into this one, as suggested by Chanwoo Choi
> v4:
> - remove unrelated changes, add newline before comment
>
> ---
> drivers/devfreq/exynos-bus.c | 130 +++++++----------------------------
> 1 file changed, 24 insertions(+), 106 deletions(-)
>
> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> index f34fa26f00d0..2aeb6cc07960 100644
> --- a/drivers/devfreq/exynos-bus.c
> +++ b/drivers/devfreq/exynos-bus.c
> @@ -25,7 +25,6 @@
> #include <linux/slab.h>
>
> #define DEFAULT_SATURATION_RATIO 40
> -#define DEFAULT_VOLTAGE_TOLERANCE 2
>
> struct exynos_bus {
> struct device *dev;
> @@ -37,9 +36,8 @@ struct exynos_bus {
>
> unsigned long curr_freq;
>
> - struct regulator *regulator;
> + struct opp_table *opp_table;
> struct clk *clk;
> - unsigned int voltage_tolerance;
> unsigned int ratio;
> };
>
> @@ -93,62 +91,29 @@ static int exynos_bus_get_event(struct exynos_bus *bus,
> }
>
> /*
> - * Must necessary function for devfreq simple-ondemand governor
> + * devfreq function for both simple-ondemand and passive governor
> */
> static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags)
> {
> struct exynos_bus *bus = dev_get_drvdata(dev);
> struct dev_pm_opp *new_opp;
> - unsigned long old_freq, new_freq, new_volt, tol;
> int ret = 0;
>
> - /* Get new opp-bus instance according to new bus clock */
> + /* Get correct frequency for bus. */
> new_opp = devfreq_recommended_opp(dev, freq, flags);
> if (IS_ERR(new_opp)) {
> dev_err(dev, "failed to get recommended opp instance\n");
> return PTR_ERR(new_opp);
> }
>
> - new_freq = dev_pm_opp_get_freq(new_opp);
> - new_volt = dev_pm_opp_get_voltage(new_opp);
> dev_pm_opp_put(new_opp);
>
> - old_freq = bus->curr_freq;
> -
> - if (old_freq == new_freq)
> - return 0;
> - tol = new_volt * bus->voltage_tolerance / 100;
> -
> /* Change voltage and frequency according to new OPP level */
> mutex_lock(&bus->lock);
> + ret = dev_pm_opp_set_rate(dev, *freq);
> + if (!ret)
> + bus->curr_freq = *freq;
>
> - if (old_freq < new_freq) {
> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
> - if (ret < 0) {
> - dev_err(bus->dev, "failed to set voltage\n");
> - goto out;
> - }
> - }
> -
> - ret = clk_set_rate(bus->clk, new_freq);
> - if (ret < 0) {
> - dev_err(dev, "failed to change clock of bus\n");
> - clk_set_rate(bus->clk, old_freq);
> - goto out;
> - }
> -
> - if (old_freq > new_freq) {
> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
> - if (ret < 0) {
> - dev_err(bus->dev, "failed to set voltage\n");
> - goto out;
> - }
> - }
> - bus->curr_freq = new_freq;
> -
> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
> - old_freq, new_freq, clk_get_rate(bus->clk));
> -out:
> mutex_unlock(&bus->lock);
>
> return ret;
> @@ -196,54 +161,10 @@ static void exynos_bus_exit(struct device *dev)
>
> dev_pm_opp_of_remove_table(dev);
> clk_disable_unprepare(bus->clk);
> - if (bus->regulator)
> - regulator_disable(bus->regulator);
> -}
> -
> -/*
> - * Must necessary function for devfreq passive governor
> - */
> -static int exynos_bus_passive_target(struct device *dev, unsigned long *freq,
> - u32 flags)
> -{
> - struct exynos_bus *bus = dev_get_drvdata(dev);
> - struct dev_pm_opp *new_opp;
> - unsigned long old_freq, new_freq;
> - int ret = 0;
> -
> - /* Get new opp-bus instance according to new bus clock */
> - new_opp = devfreq_recommended_opp(dev, freq, flags);
> - if (IS_ERR(new_opp)) {
> - dev_err(dev, "failed to get recommended opp instance\n");
> - return PTR_ERR(new_opp);
> - }
> -
> - new_freq = dev_pm_opp_get_freq(new_opp);
> - dev_pm_opp_put(new_opp);
> -
> - old_freq = bus->curr_freq;
> -
> - if (old_freq == new_freq)
> - return 0;
> -
> - /* Change the frequency according to new OPP level */
> - mutex_lock(&bus->lock);
> -
> - ret = clk_set_rate(bus->clk, new_freq);
> - if (ret < 0) {
> - dev_err(dev, "failed to set the clock of bus\n");
> - goto out;
> + if (bus->opp_table) {
> + dev_pm_opp_put_regulators(bus->opp_table);
> + bus->opp_table = NULL;
> }
> -
> - *freq = new_freq;
> - bus->curr_freq = new_freq;
> -
> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
> - old_freq, new_freq, clk_get_rate(bus->clk));
> -out:
> - mutex_unlock(&bus->lock);
> -
> - return ret;
> }
>
> static void exynos_bus_passive_exit(struct device *dev)
> @@ -258,21 +179,19 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
> struct exynos_bus *bus)
> {
> struct device *dev = bus->dev;
> + struct opp_table *opp_table;
> + const char *vdd = "vdd";
> int i, ret, count, size;
>
> - /* Get the regulator to provide each bus with the power */
> - bus->regulator = devm_regulator_get(dev, "vdd");
> - if (IS_ERR(bus->regulator)) {
> - dev_err(dev, "failed to get VDD regulator\n");
> - return PTR_ERR(bus->regulator);
> - }
> -
> - ret = regulator_enable(bus->regulator);
> - if (ret < 0) {
> - dev_err(dev, "failed to enable VDD regulator\n");
> + opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
> + if (IS_ERR(opp_table)) {
> + ret = PTR_ERR(opp_table);
> + dev_err(dev, "failed to set regulators %d\n", ret);
> return ret;
> }
>
> + bus->opp_table = opp_table;
> +
> /*
> * Get the devfreq-event devices to get the current utilization of
> * buses. This raw data will be used in devfreq ondemand governor.
> @@ -313,14 +232,11 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
> if (of_property_read_u32(np, "exynos,saturation-ratio", &bus->ratio))
> bus->ratio = DEFAULT_SATURATION_RATIO;
>
> - if (of_property_read_u32(np, "exynos,voltage-tolerance",
> - &bus->voltage_tolerance))
> - bus->voltage_tolerance = DEFAULT_VOLTAGE_TOLERANCE;
> -
> return 0;
>
> err_regulator:
> - regulator_disable(bus->regulator);
> + dev_pm_opp_put_regulators(bus->opp_table);
> + bus->opp_table = NULL;
>
> return ret;
> }
> @@ -471,7 +387,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
> goto out;
> passive:
> /* Initialize the struct profile and governor data for passive device */
> - profile->target = exynos_bus_passive_target;
> + profile->target = exynos_bus_target;
> profile->exit = exynos_bus_passive_exit;
>
> /* Get the instance of parent devfreq device */
> @@ -511,8 +427,10 @@ static int exynos_bus_probe(struct platform_device *pdev)
> dev_pm_opp_of_remove_table(dev);
> clk_disable_unprepare(bus->clk);
> err_reg:
> - if (!passive)
> - regulator_disable(bus->regulator);
> + if (!passive) {
> + dev_pm_opp_put_regulators(bus->opp_table);
> + bus->opp_table = NULL;
> + }
>
> return ret;
> }
>
It looks good to me.
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH v5 0/4] add coupled regulators for Exynos5422/5800
From: Chanwoo Choi @ 2019-08-08 1:47 UTC (permalink / raw)
To: k.konieczny
Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, Krzysztof Kozlowski,
Kukjin Kim, Kyungmin Park, Mark Rutland, MyungJoo Ham,
Nishanth Menon, Rob Herring, Stephen Boyd, Viresh Kumar,
devicetree, linux-arm-kernel, linux-kernel, linux-pm,
linux-samsung-soc
In-Reply-To: <20190807133838.14678-1-k.konieczny@partner.samsung.com>
Hi Kamil,
When I applied them to testing branch, those don't have the author name
only just have the email address as following:
You have to edit the your git author information with your name.
author k.konieczny@partner.samsung.com <k.konieczny@partner.samsung.com> 2019-08-07 15:38:36 +0200
committer Chanwoo Choi <cw00.choi@samsung.com> 2019-08-08 10:35:16 +0900
commit 4304f4ecec93cebd255463d56b0a4f112ee9dc50 (patch)
tree 2859e566d6f68219f71a61e7c412717c1adba4f5
parent 57d85421038b458dd87ec268404ff608f90c36ae (diff)
download linux-4304f4ecec93cebd255463d56b0a4f112ee9dc50.tar.gz
Regards,
Chanwoo Choi
On 19. 8. 7. 오후 10:38, k.konieczny@partner.samsung.com wrote:
> Hi,
>
> The main purpose of this patch series is to add coupled regulators for
> Exynos5422/5800 to keep constrain on voltage difference between vdd_arm
> and vdd_int to be at most 300mV. In exynos-bus instead of using
> regulator_set_voltage_tol() with default voltage tolerance it should be
> used regulator_set_voltage_triplet() with volatege range, and this is
> already present in opp/core.c code, so it can be reused. While at this,
> move setting regulators into opp/core.
>
> This patchset was tested on Odroid XU3.
>
> The DTS coupled regulators patch depends on previous patches.
>
> Changes:
> v5:
> - squashed last patch "remove exynos_bus_passive_target()" into second
> - added Acked-by to patch "correct clock enable sequence"
> v4:
> - removed "opp: core: add regulators enable and disable" from patchset
> as it was applied by Viresh Kumar and changed cover letter
> - fix patch "devfreq: exynos-bus: correct clock enable sequence" to
> correct order of enable/disable
> - removed unrelated changes in "devfreq: exynos-bus: convert to use
> dev_pm_opp_set_rate()"
> - added new patch "devfreq: exynos-bus: remove exynos_bus_passive_target()"
> as suggested by Chanwoo Choi
> v3:
> - added new exynos-bus patch to correct clock and regulator enabling
> and disabling sequence as suggested by Chanwoo Choi
> - corrected error path in enable and improved commit message in opp/core
> - improve comment in devfreq/exynos-bus.c before devfreq_recommended_opp()
> - change cover letter as there is new patch
> - added note before Signed-off-by in 4th patch
> v2:
> - improve regulators enable/disable code in opp/core as suggested by
> Viresh Kumar
> - add new patch for remove unused dt-bindings as suggested by Krzysztof
> Kozlowski
>
> Kamil Konieczny (3):
> devfreq: exynos-bus: correct clock enable sequence
> devfreq: exynos-bus: convert to use dev_pm_opp_set_rate()
> dt-bindings: devfreq: exynos-bus: remove unused property
>
> Marek Szyprowski (1):
> ARM: dts: exynos: add initial data for coupled regulators for
> Exynos5422/5800
>
> .../bindings/devfreq/exynos-bus.txt | 2 -
> arch/arm/boot/dts/exynos5420.dtsi | 34 ++--
> arch/arm/boot/dts/exynos5422-odroid-core.dtsi | 4 +
> arch/arm/boot/dts/exynos5800-peach-pi.dts | 4 +
> arch/arm/boot/dts/exynos5800.dtsi | 32 ++--
> drivers/devfreq/exynos-bus.c | 153 +++++-------------
> 6 files changed, 78 insertions(+), 151 deletions(-)
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH v7 0/7] Solve postboot supplier cleanup and optimize probe ordering
From: Frank Rowand @ 2019-08-08 2:02 UTC (permalink / raw)
To: Saravana Kannan, Rob Herring, Mark Rutland, Greg Kroah-Hartman,
Rafael J. Wysocki
Cc: devicetree, linux-kernel, David Collins, kernel-team
In-Reply-To: <20190724001100.133423-1-saravanak@google.com>
Hi Saravana,
On 7/23/19 5:10 PM, Saravana Kannan wrote:
> Add device-links to track functional dependencies between devices
> after they are created (but before they are probed) by looking at
> their common DT bindings like clocks, interconnects, etc.
>
< snip >
I know that this series has moved on to versions 8 and 9. And some
additional patches submitted.
Version 8 was a rebase to handle device_link changes. The version 8
patch 0/7 description of the changes did not note any functional
changes, so I am assuming that my comments on version 7 are still
applicable.
The version 9 changes do not impact my comments.
I am sending review comments on patches 1, 2, and 3. I will continue
review of patches later in the series when the fall out from these
review comments result in a new series.
-Frank
^ permalink raw reply
* Re: [PATCH v7 1/7] driver core: Add support for linking devices during device addition
From: Frank Rowand @ 2019-08-08 2:04 UTC (permalink / raw)
To: Saravana Kannan, Rob Herring, Mark Rutland, Greg Kroah-Hartman,
Rafael J. Wysocki
Cc: devicetree, linux-kernel, David Collins, kernel-team
In-Reply-To: <20190724001100.133423-2-saravanak@google.com>
> Date: Tue, 23 Jul 2019 17:10:54 -0700
> Subject: [PATCH v7 1/7] driver core: Add support for linking devices during
> device addition
> From: Saravana Kannan <saravanak@google.com>
>
> When devices are added, the bus might want to create device links to track
> functional dependencies between supplier and consumer devices. This
> tracking of supplier-consumer relationship allows optimizing device probe
> order and tracking whether all consumers of a supplier are active. The
> add_links bus callback is added to support this.
Change above to:
When devices are added, the bus may create device links to track which
suppliers a consumer device depends upon. This
tracking of supplier-consumer relationship may be used to defer probing
the driver of a consumer device before the driver(s) for its supplier device(s)
are probed. It may also be used by a supplier driver to determine if
all of its consumers have been successfully probed.
The add_links bus callback is added to create the supplier device links
>
> However, when consumer devices are added, they might not have a supplier
> device to link to despite needing mandatory resources/functionality from
> one or more suppliers. A waiting_for_suppliers list is created to track
> such consumers and retry linking them when new devices get added.
Change above to:
If a supplier device has not yet been created when the consumer device attempts
to link it, the consumer device is added to the wait_for_suppliers list.
When supplier devices are created, the supplier device link will be added to
the relevant consumer devices on the wait_for_suppliers list.
>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
> drivers/base/core.c | 83 ++++++++++++++++++++++++++++++++++++++++++
> include/linux/device.h | 14 +++++++
> 2 files changed, 97 insertions(+)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index da84a73f2ba6..1b4eb221968f 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -44,6 +44,8 @@ early_param("sysfs.deprecated", sysfs_deprecated_setup);
> #endif
>
> /* Device links support. */
> +static LIST_HEAD(wait_for_suppliers);
> +static DEFINE_MUTEX(wfs_lock);
>
> #ifdef CONFIG_SRCU
> static DEFINE_MUTEX(device_links_lock);
> @@ -401,6 +403,51 @@ struct device_link *device_link_add(struct device *consumer,
> }
> EXPORT_SYMBOL_GPL(device_link_add);
>
> +/**
> + * device_link_wait_for_supplier - Mark device as waiting for supplier
* device_link_wait_for_supplier - Add device to wait_for_suppliers list
> + * @consumer: Consumer device
> + *
> + * Marks the consumer device as waiting for suppliers to become available. The
> + * consumer device will never be probed until it's unmarked as waiting for
> + * suppliers. The caller is responsible for adding the link to the supplier
> + * once the supplier device is present.
> + *
> + * This function is NOT meant to be called from the probe function of the
> + * consumer but rather from code that creates/adds the consumer device.
> + */
> +static void device_link_wait_for_supplier(struct device *consumer)
> +{
> + mutex_lock(&wfs_lock);
> + list_add_tail(&consumer->links.needs_suppliers, &wait_for_suppliers);
> + mutex_unlock(&wfs_lock);
> +}
> +
> +/**
> + * device_link_check_waiting_consumers - Try to remove from supplier wait list
> + *
> + * Loops through all consumers waiting on suppliers and tries to add all their
> + * supplier links. If that succeeds, the consumer device is unmarked as waiting
> + * for suppliers. Otherwise, they are left marked as waiting on suppliers,
> + *
> + * The add_links bus callback is expected to return 0 if it has found and added
> + * all the supplier links for the consumer device. It should return an error if
> + * it isn't able to do so.
> + *
> + * The caller of device_link_wait_for_supplier() is expected to call this once
> + * it's aware of potential suppliers becoming available.
Change above comment to:
* device_link_add_supplier_links - add links from consumer devices to
* supplier devices, leaving any consumer
* with inactive suppliers on the
* wait_for_suppliers list
* Scan all consumer devices in the devicetree. For any supplier device that
* is not already linked to the consumer device, add the supplier to the
* consumer device's device links.
*
* If all of a consumer device's suppliers are available then the consumer
* is removed from the wait_for_suppliers list (if previously on the list).
* Otherwise the consumer is added to the wait_for_suppliers list (if not
* already on the list).
* The add_links bus callback must return 0 if it has found and added all
* the supplier links for the consumer device. It must return an error if
* it is not able to do so.
*
* The caller of device_link_wait_for_supplier() is expected to call this once
* it is aware of potential suppliers becoming available.
> + */
> +static void device_link_check_waiting_consumers(void)
Function name is misleading and hides side effects.
I have not come up with a name that does not hide side effects, but a better
name would be:
device_link_add_supplier_links()
> +{
> + struct device *dev, *tmp;
> +
> + mutex_lock(&wfs_lock);
> + list_for_each_entry_safe(dev, tmp, &wait_for_suppliers,
> + links.needs_suppliers)
> + if (!dev->bus->add_links(dev))
> + list_del_init(&dev->links.needs_suppliers);
Empties dev->links.needs_suppliers, but does not remove dev from
wait_for_suppliers list. Where does that happen?
> + mutex_unlock(&wfs_lock);
> +}
> +
> static void device_link_free(struct device_link *link)
> {
> while (refcount_dec_not_one(&link->rpm_active))
> @@ -535,6 +582,19 @@ int device_links_check_suppliers(struct device *dev)
> struct device_link *link;
> int ret = 0;
>
> + /*
> + * If a device is waiting for one or more suppliers (in
> + * wait_for_suppliers list), it is not ready to probe yet. So just
> + * return -EPROBE_DEFER without having to check the links with existing
> + * suppliers.
> + */
Change comment to:
/*
* Device waiting for supplier to become available is not allowed
* to probe
*/
> + mutex_lock(&wfs_lock);
> + if (!list_empty(&dev->links.needs_suppliers)) {
> + mutex_unlock(&wfs_lock);
> + return -EPROBE_DEFER;
> + }
> + mutex_unlock(&wfs_lock);
> +
> device_links_write_lock();
Update Documentation/driver-api/device_link.rst to reflect the
check of &dev->links.needs_suppliers in device_links_check_suppliers().
>
> list_for_each_entry(link, &dev->links.suppliers, c_node) {
> @@ -812,6 +872,10 @@ static void device_links_purge(struct device *dev)
> {
> struct device_link *link, *ln;
>
> + mutex_lock(&wfs_lock);
> + list_del(&dev->links.needs_suppliers);
> + mutex_unlock(&wfs_lock);
> +
> /*
> * Delete all of the remaining links from this device to any other
> * devices (either consumers or suppliers).
> @@ -1673,6 +1737,7 @@ void device_initialize(struct device *dev)
> #endif
> INIT_LIST_HEAD(&dev->links.consumers);
> INIT_LIST_HEAD(&dev->links.suppliers);
> + INIT_LIST_HEAD(&dev->links.needs_suppliers);
> dev->links.status = DL_DEV_NO_DRIVER;
> }
> EXPORT_SYMBOL_GPL(device_initialize);
> @@ -2108,6 +2173,24 @@ int device_add(struct device *dev)
> BUS_NOTIFY_ADD_DEVICE, dev);
>
> kobject_uevent(&dev->kobj, KOBJ_ADD);
> +
> + /*
> + * Check if any of the other devices (consumers) have been waiting for
> + * this device (supplier) to be added so that they can create a device
> + * link to it.
> + *
> + * This needs to happen after device_pm_add() because device_link_add()
> + * requires the supplier be registered before it's called.
> + *
> + * But this also needs to happe before bus_probe_device() to make sure
> + * waiting consumers can link to it before the driver is bound to the
> + * device and the driver sync_state callback is called for this device.
> + */
/*
* Add links to dev from any dependent consumer that has dev on it's
* list of needed suppliers (links.needs_suppliers). Device_pm_add()
* must have previously registered dev to allow the links to be added.
*
* The consumer links must be created before dev is probed because the
* sync_state callback for dev will use the consumer links.
*/
> + device_link_check_waiting_consumers();
> +
> + if (dev->bus && dev->bus->add_links && dev->bus->add_links(dev))
> + device_link_wait_for_supplier(dev);
> +
> bus_probe_device(dev);
> if (parent)
> klist_add_tail(&dev->p->knode_parent,
> diff --git a/include/linux/device.h b/include/linux/device.h
> index c330b75c6c57..5d70babb7462 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -78,6 +78,17 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
> * -EPROBE_DEFER it will queue the device for deferred probing.
> * @uevent: Called when a device is added, removed, or a few other things
> * that generate uevents to add the environment variables.
> + * @add_links: Called, perhaps multiple times per device, after a device is
> + * added to this bus. The function is expected to create device
> + * links to all the suppliers of the input device that are
> + * available at the time this function is called. As in, the
> + * function should NOT stop at the first failed device link if
> + * other unlinked supplier devices are present in the system.
* @add_links: Called after a device is added to this bus. The function is
* expected to create device links to all the suppliers of the
* device that are available at the time this function is called.
* The function must NOT stop at the first failed device link if
* other unlinked supplier devices are present in the system.
* If some suppliers are not yet available, this function will be
* called again when the suppliers become available.
but add_links() not needed, so moving this comment to of_link_to_suppliers()
> + *
> + * Return 0 if device links have been successfully created to all
> + * the suppliers of this device. Return an error if some of the
> + * suppliers are not yet available and this function needs to be
> + * reattempted in the future.
*
* Return 0 if device links have been successfully created to all
* the suppliers of this device. Return an error if some of the
* suppliers are not yet available.
> * @probe: Called when a new device or driver add to this bus, and callback
> * the specific driver's probe to initial the matched device.
> * @remove: Called when a device removed from this bus.
> @@ -122,6 +133,7 @@ struct bus_type {
>
> int (*match)(struct device *dev, struct device_driver *drv);
> int (*uevent)(struct device *dev, struct kobj_uevent_env *env);
> + int (*add_links)(struct device *dev);
^^^^^^^^^ add_supplier ???
^^^^^^^^^ add_suppliers ???
^^^^^^^^^ link_suppliers ???
^^^^^^^^^ add_supplier_dependency ???
^^^^^^^^^ add_supplier_dependencies ???
add_links() not needed
> int (*probe)(struct device *dev);
> int (*remove)(struct device *dev);
> void (*shutdown)(struct device *dev);
> @@ -893,11 +905,13 @@ enum dl_dev_state {
> * struct dev_links_info - Device data related to device links.
> * @suppliers: List of links to supplier devices.
> * @consumers: List of links to consumer devices.
> + * @needs_suppliers: Hook to global list of devices waiting for suppliers.
* @needs_suppliers: List of devices deferring probe until supplier drivers
* are successfully probed.
> * @status: Driver status information.
> */
> struct dev_links_info {
> struct list_head suppliers;
> struct list_head consumers;
> + struct list_head needs_suppliers;
> enum dl_dev_state status;
> };
>
> --
> 2.22.0.709.g102302147b-goog
>
>
^ permalink raw reply
* Re: [PATCH v7 2/7] driver core: Add edit_links() callback for drivers
From: Frank Rowand @ 2019-08-08 2:05 UTC (permalink / raw)
To: Saravana Kannan, Rob Herring, Mark Rutland, Greg Kroah-Hartman,
Rafael J. Wysocki
Cc: devicetree, linux-kernel, David Collins, kernel-team
In-Reply-To: <20190724001100.133423-3-saravanak@google.com>
> Date: Tue, 23 Jul 2019 17:10:55 -0700
> Subject: [PATCH v7 2/7] driver core: Add edit_links() callback for drivers
> From: Saravana Kannan <saravanak@google.com>
>
> The driver core/bus adding supplier-consumer dependencies by default
> enables functional dependencies to be tracked correctly even when the
> consumer devices haven't had their drivers registered or loaded (if they
> are modules).
enables functional dependencies to be tracked correctly before the
consumer device drivers are registered or loaded (if they are modules).
>
> However, when the bus incorrectly adds dependencies that it shouldn't
^^^ driver core/bus
> have added, the devices might never probe.
Explain what causes a dependency to be incorrectly added.
Is this a bug in the dependency detection code?
Are there cases where the dependency detection code can not reliably determine
whether there truly is a dependency?
>
> For example, if device-C is a consumer of device-S and they have
> phandles to each other in DT, the following could happen:
>
> 1. Device-S get added first.
> 2. The bus add_links() callback will (incorrectly) try to link it as
> a consumer of device-C.
> 3. Since device-C isn't present, device-S will be put in
> "waiting-for-supplier" list.
> 4. Device-C gets added next.
> 5. All devices in "waiting-for-supplier" list are retried for linking.
> 6. Device-S gets linked as consumer to Device-C.
> 7. The bus add_links() callback will (correctly) try to link it as
> a consumer of device-S.
> 8. This isn't allowed because it would create a cyclic device links.
>
> Neither devices will get probed since the supplier is marked as
> dependent on the consumer. And the consumer will never probe because the
> consumer can't get resources from the supplier.
>
> Without this patch, things stay in this broken state. However, with this
> patch, the execution will continue like this:
>
> 9. Device-C's driver is loaded.
Change comment to:
For example, if device-C is a consumer of device-S and they have phandles
referencing each other in the devicetree, the following could happen:
1. Device-S is added first.
- The bus add_links() callback will (incorrectly) link device-S
as a consumer of device-C, and device-S will be put in the
"wait_for_suppliers" list.
2. Device-C is added next.
- All devices in the "wait_for_suppliers" list are retried for linking.
- Device-S remains linked as a consumer to device-C.
- The bus add_links() callback will (correctly) try to link device-C as
a consumer of device-S.
- The link attempt will fail because it would create a cyclic device
link, and device-C will be put in the "wait_for_suppliers" list.
Device-S will not be probed because it is in the "wait_for_suppliers" list.
Device-C will not be probed because it is in the "wait_for_suppliers" list.
>
> Without this patch, things stay in this broken state. However, with this
> patch, the execution will continue like this:
>
> 9. Device-C's driver is loaded.
What is "loaded"? Does that mean the device-C probe succeeds?
What causes device-C to be probed? The normal processing of -EPROBE_DEFER
devices?
> 10. Device-C's driver removes Device-S as a consumer of Device-C.
> 11. Device-C's driver adds Device-C as a consumer of Device-S.
> 12. Device-S probes.
> 14. Device-C probes.
>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
> drivers/base/core.c | 24 ++++++++++++++++++++++--
> drivers/base/dd.c | 29 +++++++++++++++++++++++++++++
> include/linux/device.h | 18 ++++++++++++++++++
> 3 files changed, 69 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 1b4eb221968f..733d8a9aec76 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -422,6 +422,19 @@ static void device_link_wait_for_supplier(struct device *consumer)
> mutex_unlock(&wfs_lock);
> }
>
> +/**
> + * device_link_remove_from_wfs - Unmark device as waiting for supplier
> + * @consumer: Consumer device
> + *
> + * Unmark the consumer device as waiting for suppliers to become available.
> + */
> +void device_link_remove_from_wfs(struct device *consumer)
Misleading function name.
Incorrect description.
Does not remove consumer from list wait_for_suppliers.
At best, consumer might eventually get removed from list wait_for_suppliers
if device_link_check_waiting_consumers() is called again.
> +{
> + mutex_lock(&wfs_lock);
> + list_del_init(&consumer->links.needs_suppliers);
> + mutex_unlock(&wfs_lock);
> +}
> +
> /**
> * device_link_check_waiting_consumers - Try to unmark waiting consumers
> *
> @@ -439,12 +452,19 @@ static void device_link_wait_for_supplier(struct device *consumer)
> static void device_link_check_waiting_consumers(void)
> {
> struct device *dev, *tmp;
> + int ret;
>
> mutex_lock(&wfs_lock);
> list_for_each_entry_safe(dev, tmp, &wait_for_suppliers,
> - links.needs_suppliers)
> - if (!dev->bus->add_links(dev))
> + links.needs_suppliers) {
> + ret = 0;
> + if (dev->has_edit_links)
> + ret = driver_edit_links(dev);
> + else if (dev->bus->add_links)
> + ret = dev->bus->add_links(dev);
> + if (!ret)
> list_del_init(&dev->links.needs_suppliers);
> + }
> mutex_unlock(&wfs_lock);
> }
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 994a90747420..5e7041ede0d7 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -698,6 +698,12 @@ int driver_probe_device(struct device_driver *drv, struct device *dev)
> pr_debug("bus: '%s': %s: matched device %s with driver %s\n",
> drv->bus->name, __func__, dev_name(dev), drv->name);
>
> + if (drv->edit_links) {
> + if (drv->edit_links(dev))
> + dev->has_edit_links = true;
> + else
> + device_link_remove_from_wfs(dev);
> + }
For the purposes of the following paragraphs, I refer to dev as "dev_1" to
distinguish it from a new dev that will be encountered later. The following
paragraphs assume dev_1 has a supplier dependency for a supplier that has
not probed yet.
Q. Why the extra level of indirection?
A. really_probe() does not set dev->driver before returning if
device_links_check_suppliers() returned -EPROBE_DEFER. Thus
device_link_check_waiting_consumers() can not directly check
"if (dev_1->driver->edit_links)".
The added driver_probe_device() code is setting dev_1->has_edit_links in the
probe path, then device_link_check_waiting_consumers() will use the value
of dev_1->has_edit_links instead of directly checking
"if (dev_1->driver->edit_links)".
If really_probe() was modified to set dev->driver in this
case then the need for dev->has_edit_links is removed and
driver_edit_links() is not needed, since dev->driver would
be available. Removing driver_edit_links() simplifies the
code.
device_add() calls dev_1->bus->add_links(dev_1), thus dev_1 will have the
supplier links set (for any suppliers not currently available) and be on
list wait_for_suppliers.
Then device_add() calls bus_probe_device(), leading to calling
driver_probe_device(). The above code fragment either sets
dev_1->has_edit_links or removes the needs_suppliers links from dev_1.
dev_1 remains on list wait_for_suppliers.
If (drv->edit_links(dev_1) returns 0 then device_link_remove_from_wfs()
removes the supplier links. Shouldn't device_link_remove_from_wfs() also
remove the device from the list wait_for_suppliers?
The next time a device is added, device_link_check_waiting_consumers() will
be called and dev_1 will be on list wait_for_suppliers, thus
device_link_check_waiting_consumers() will find dev_1->has_edit_links true
and thus call driver_edit_links() instead of calling dev->bus->add_links().
The comment in device.h, later in this patch, says that drv->edit_links() is
responsible for editing the device links for dev. The comment provides no
guidance on how drv->edit_links() is supposed to determine what edits to
perform. No example drv->edit_links() function is provided in this patch
series. dev_1->bus->add_links(dev_1) may have added one or more suppliers
to its needs_suppliers link. drv->edit_links() needs to be able to handle
all possible variants of what suppliers are on the needs_suppliers link.
> pm_runtime_get_suppliers(dev);
> if (dev->parent)
> pm_runtime_get_sync(dev->parent);
> @@ -786,6 +792,29 @@ struct device_attach_data {
> bool have_async;
> };
>
> +static int __driver_edit_links(struct device_driver *drv, void *data)
> +{
> + struct device *dev = data;
> +
> + if (!drv->edit_links)
> + return 0;
> +
> + if (driver_match_device(drv, dev) <= 0)
> + return 0;
> +
> + return drv->edit_links(dev);
> +}
> +
> +int driver_edit_links(struct device *dev)
> +{
> + int ret;
> +
> + device_lock(dev);
> + ret = bus_for_each_drv(dev->bus, NULL, dev, __driver_edit_links);
> + device_unlock(dev);
> + return ret;
> +}
> +
> static int __device_attach_driver(struct device_driver *drv, void *_data)
> {
> struct device_attach_data *data = _data;
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 5d70babb7462..35aed50033c4 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -263,6 +263,20 @@ enum probe_type {
> * @probe_type: Type of the probe (synchronous or asynchronous) to use.
> * @of_match_table: The open firmware table.
> * @acpi_match_table: The ACPI match table.
> + * @edit_links: Called to allow a matched driver to edit the device links the
Where is the value of field edit_links set?
Is it only in an out of tree driver? If so, I would like to see an
example implementation of the edit_links() function.
> + * bus might have added incorrectly. This will be useful to handle
> + * cases where the bus incorrectly adds functional dependencies
> + * that aren't true or tries to create cyclic dependencies. But
> + * doesn't correctly handle functional dependencies that are
> + * missed by the bus as the supplier's sync_state might get to
> + * execute before the driver for a missing consumer is loaded and
> + * gets to edit the device links for the consumer.
> + *
> + * This function might be called multiple times after a new device
> + * is added. The function is expected to create all the device
> + * links for the new device and return 0 if it was completed
> + * successfully or return an error if it needs to be reattempted
> + * in the future.
> * @probe: Called to query the existence of a specific device,
> * whether this driver can work with it, and bind the driver
> * to a specific device.
> @@ -302,6 +316,7 @@ struct device_driver {
> const struct of_device_id *of_match_table;
> const struct acpi_device_id *acpi_match_table;
>
> + int (*edit_links)(struct device *dev);
> int (*probe) (struct device *dev);
> int (*remove) (struct device *dev);
> void (*shutdown) (struct device *dev);
> @@ -1078,6 +1093,7 @@ struct device {
> bool offline_disabled:1;
> bool offline:1;
> bool of_node_reused:1;
> + bool has_edit_links:1;
Add has_edit_links to the struct's kernel_doc
> #if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
> defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
> defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
> @@ -1329,6 +1345,7 @@ extern int __must_check device_attach(struct device *dev);
> extern int __must_check driver_attach(struct device_driver *drv);
> extern void device_initial_probe(struct device *dev);
> extern int __must_check device_reprobe(struct device *dev);
> +extern int driver_edit_links(struct device *dev);
>
> extern bool device_is_bound(struct device *dev);
>
> @@ -1419,6 +1436,7 @@ struct device_link *device_link_add(struct device *consumer,
> struct device *supplier, u32 flags);
> void device_link_del(struct device_link *link);
> void device_link_remove(void *consumer, struct device *supplier);
> +void device_link_remove_from_wfs(struct device *consumer);
>
> #ifndef dev_fmt
> #define dev_fmt(fmt) fmt
> --
> 2.22.0.709.g102302147b-goog
>
>
^ permalink raw reply
* Re: [PATCH v7 3/7] of/platform: Add functional dependency link from DT bindings
From: Frank Rowand @ 2019-08-08 2:06 UTC (permalink / raw)
To: Saravana Kannan, Rob Herring, Mark Rutland, Greg Kroah-Hartman,
Rafael J. Wysocki, Jonathan Corbet
Cc: devicetree, linux-kernel, David Collins, kernel-team, linux-doc
In-Reply-To: <20190724001100.133423-4-saravanak@google.com>
On 7/23/19 5:10 PM, Saravana Kannan wrote:
> Add device-links after the devices are created (but before they are
> probed) by looking at common DT bindings like clocks and
> interconnects.
>
> Automatically adding device-links for functional dependencies at the
> framework level provides the following benefits:
>
> - Optimizes device probe order and avoids the useless work of
> attempting probes of devices that will not probe successfully
> (because their suppliers aren't present or haven't probed yet).
>
> For example, in a commonly available mobile SoC, registering just
> one consumer device's driver at an initcall level earlier than the
> supplier device's driver causes 11 failed probe attempts before the
> consumer device probes successfully. This was with a kernel with all
> the drivers statically compiled in. This problem gets a lot worse if
> all the drivers are loaded as modules without direct symbol
> dependencies.
>
> - Supplier devices like clock providers, interconnect providers, etc
> need to keep the resources they provide active and at a particular
> state(s) during boot up even if their current set of consumers don't
> request the resource to be active. This is because the rest of the
> consumers might not have probed yet and turning off the resource
> before all the consumers have probed could lead to a hang or
> undesired user experience.
>
> Some frameworks (Eg: regulator) handle this today by turning off
> "unused" resources at late_initcall_sync and hoping all the devices
> have probed by then. This is not a valid assumption for systems with
> loadable modules. Other frameworks (Eg: clock) just don't handle
> this due to the lack of a clear signal for when they can turn off
> resources. This leads to downstream hacks to handle cases like this
> that can easily be solved in the upstream kernel.
>
> By linking devices before they are probed, we give suppliers a clear
> count of the number of dependent consumers. Once all of the
> consumers are active, the suppliers can turn off the unused
> resources without making assumptions about the number of consumers.
>
> By default we just add device-links to track "driver presence" (probe
> succeeded) of the supplier device. If any other functionality provided
> by device-links are needed, it is left to the consumer/supplier
> devices to change the link when they probe.
>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
> .../admin-guide/kernel-parameters.txt | 5 +
> drivers/of/platform.c | 165 ++++++++++++++++++
> 2 files changed, 170 insertions(+)
>
Documentation/admin-guide/kernel-paramers.rst:
After line 129, add:
OF Devicetree is enabled
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 46b826fcb5ad..12937349d79d 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3170,6 +3170,11 @@
> This can be set from sysctl after boot.
> See Documentation/admin-guide/sysctl/vm.rst for details.
>
> + of_devlink [KNL] Make device links from common DT bindings. Useful
> + for optimizing probe order and making sure resources
> + aren't turned off before the consumer devices have
> + probed.
of_supplier_depend instead of of_devlink ????
of_supplier_depend
[OF, KNL] Make device links from consumer devicetree
nodes to supplier devicetree nodes. The
consumer / supplier relationships are inferred from
scanning the devicetree. The driver for a consumer
device will not be probed until the drivers for all of
its supplier devices have been successfully probed.
> +
> ohci1394_dma=early [HW] enable debugging via the ohci1394 driver.
> See Documentation/debugging-via-ohci1394.txt for more
> info.
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 7801e25e6895..4344419a26fc 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -508,6 +508,170 @@ int of_platform_default_populate(struct device_node *root,
> }
> EXPORT_SYMBOL_GPL(of_platform_default_populate);
>
> +bool of_link_is_valid(struct device_node *con, struct device_node *sup)
Change to less vague:
bool of_ancestor_of(struct device_node *test_np, struct device_node *np)
> +{
> + of_node_get(sup);
> + /*
> + * Don't allow linking a device node as a consumer of one of its
> + * descendant nodes. By definition, a child node can't be a functional
> + * dependency for the parent node.
> + */
> + while (sup) {
> + if (sup == con) {
> + of_node_put(sup);
> + return false;
> + }
> + sup = of_get_next_parent(sup);
> + }
> + return true;
Change to more generic:
of_node_get(test_np);
while (test_np) {
if (test_np == np) {
of_node_put(test_np);
return true;
}
test_np = of_get_next_parent(test_np);
}
return false;
> +}
> +
/**
* of_link_to_phandle - Add device link to supplier
* @dev: consumer device
* @sup_np: pointer to the supplier device tree node
*
* TODO: ...
*
* Return:
* * 0 if link successfully created for supplier or of_devlink is false
* * an error if unable to create link
*/
Should have dev_debug() or pr_warn() or something on errors in this
function -- the caller does not report any issue
> +static int of_link_to_phandle(struct device *dev, struct device_node *sup_np)
> +{
> + struct platform_device *sup_dev;
> + u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
> + int ret = 0;
> +
> + /*
> + * Since we are trying to create device links, we need to find
> + * the actual device node that owns this supplier phandle.
> + * Often times it's the same node, but sometimes it can be one
> + * of the parents. So walk up the parent till you find a
> + * device.
Change comment to:
* Find the device node that contains the supplier phandle. It may
* be @sup_np or it may be an ancestor of @sup_np.
> + */
See comment in caller of of_link_to_phandle() - do not hide the final
of_node_put() of sup_np inside of_link_to_phandle(), so need to do
an of_node_get() here.
of_node_get(sup_np);
> + while (sup_np && !of_find_property(sup_np, "compatible", NULL))
> + sup_np = of_get_next_parent(sup_np);
> + if (!sup_np)
> + return 0;
This case should never occur(?), it is an error.
return -ENODEV;
> +
> + if (!of_link_is_valid(dev->of_node, sup_np)) {
> + of_node_put(sup_np);
> + return 0;
Do not use a name that obscures what the function is doing, also
return an actual issue.
if (of_ancestor_of(sup_np, dev->of_node)) {
of_node_put(sup_np);
return -EINVAL;
> + }
> + sup_dev = of_find_device_by_node(sup_np);
> + of_node_put(sup_np);
> + if (!sup_dev)
> + return -ENODEV;
> + if (!device_link_add(dev, &sup_dev->dev, dl_flags))
> + ret = -ENODEV;
> + put_device(&sup_dev->dev);
> + return ret;
> +}
> +
/**
* parse_prop_cells - Property parsing functions for suppliers
*
* @np: pointer to a device tree node containing a list
* @prop_name: Name of property holding a phandle value
* @phandle_index: For properties holding a table of phandles, this is the
* index into the table
* @list_name: property name that contains a list
* @cells_name: property name that specifies phandles' arguments count
*
* This function is useful to parse lists of phandles and their arguments.
*
* Return:
* * Node pointer with refcount incremented, use of_node_put() on it when done.
* * NULL if not found.
*/
> +static struct device_node *parse_prop_cells(struct device_node *np,
> + const char *prop, int index,
> + const char *binding,
> + const char *cell)
Make names consistent with of_parse_phandle_with_args():
Change prop to prop_name
Change index to phandle_index
Change binding to list_name
Change cell to cells_name
> +{
> + struct of_phandle_args sup_args;
> +
> + /* Don't need to check property name for every index. */
> + if (!index && strcmp(prop, binding))
> + return NULL;
I read the discussion on whether to check property name only once
in version 6.
This check is fragile, depending upon the calling code to be properly
structured. Do the check for all values of index. The reduction of
overhead from not checking does not justify the fragileness and the
extra complexity for the code reader to understand why the check can
be bypassed when
index is not zero.
> +
> + if (of_parse_phandle_with_args(np, binding, cell, index, &sup_args))
> + return NULL;
> +
> + return sup_args.np;
> +}
> +
> +static struct device_node *parse_clocks(struct device_node *np,
> + const char *prop, int index)
Change prop to prop_name
Change index to phandle_index
> +{
> + return parse_prop_cells(np, prop, index, "clocks", "#clock-cells");
> +}
> +
> +static struct device_node *parse_interconnects(struct device_node *np,
> + const char *prop, int index)
Change prop to prop_name
Change index to phandle_index
> +{
> + return parse_prop_cells(np, prop, index, "interconnects",
> + "#interconnect-cells");
> +}
> +
> +static int strcmp_suffix(const char *str, const char *suffix)
> +{
> + unsigned int len, suffix_len;
> +
> + len = strlen(str);
> + suffix_len = strlen(suffix);
> + if (len <= suffix_len)
> + return -1;
> + return strcmp(str + len - suffix_len, suffix);
> +}
> +
> +static struct device_node *parse_regulators(struct device_node *np,
> + const char *prop, int index)
Change prop to prop_name
Change index to phandle_index
> +{
> + if (index || strcmp_suffix(prop, "-supply"))
> + return NULL;
> +
> + return of_parse_phandle(np, prop, 0);
> +}
> +
> +/**
> + * struct supplier_bindings - Information for parsing supplier DT binding
> + *
> + * @parse_prop: If the function cannot parse the property, return NULL.
> + * Otherwise, return the phandle listed in the property
> + * that corresponds to the index.
There is no documentation of dynamic function parameters in the docbook
description of a struct. Use this format for now and I will clean up when
I clean up all of the devicetree docbook info.
Change above comment to:
* struct supplier_bindings - Property parsing functions for suppliers
*
* @parse_prop: function name
* parse_prop() finds the node corresponding to a supplier phandle
* @parse_prop.np: Pointer to device node holding supplier phandle property
* @parse_prop.prop_name: Name of property holding a phandle value
* @parse_prop.index: For properties holding a table of phandles, this is the
* index into the table
*
* Return:
* * parse_prop() return values are
* * Node pointer with refcount incremented, use of_node_put() on it when done.
* * NULL if not found.
> + */
> +struct supplier_bindings {
> + struct device_node *(*parse_prop)(struct device_node *np,
> + const char *name, int index);
Change name to prop_name
Change index to phandle_index
> +};
> +
> +static const struct supplier_bindings bindings[] = {
> + { .parse_prop = parse_clocks, },
> + { .parse_prop = parse_interconnects, },
> + { .parse_prop = parse_regulators, },
> + { },
{},
> +};
> +
/**
* of_link_property - TODO:
* dev:
* con_np:
* prop:
*
* TODO...
*
* Any failed attempt to create a link will NOT result in an immediate return.
* of_link_property() must create all possible links even when one of more
* attempts to create a link fail.
Why? isn't one failure enough to prevent probing this device?
Continuing to scan just results in extra work... which will be
repeated every time device_link_check_waiting_consumers() is called
*
* Return:
* * 0 if TODO:
* * -ENODEV on error
*/
I left some "TODO:" sections to be filled out above.
> +static bool of_link_property(struct device *dev, struct device_node *con_np,
> + const char *prop)
Returns 0 or -ENODEV, so bool is incorrect
(Also fixed on 8/8 in patch: "[PATCH 1/2] of/platform: Fix fn definitons for
of_link_is_valid() and of_link_property()")
> +{
> + struct device_node *phandle;
> + struct supplier_bindings *s = bindings;
> + unsigned int i = 0;
> + bool done = true, matched = false;
Change to:
bool matched = false;
int ret = 0;
/* do not stop at first failed link, link all available suppliers */
> +
> + while (!matched && s->parse_prop) {
> + while ((phandle = s->parse_prop(con_np, prop, i))) {
> + matched = true;
> + i++;
> + if (of_link_to_phandle(dev, phandle))
Remove comment:
> + /*
> + * Don't stop at the first failure. See
> + * Documentation for bus_type.add_links for
> + * more details.
> + */
> + done = false;
ret = -ENODEV;
Do not hide of_node_put() inside of_link_to_phandle(), do it here:
of_node_put(phandle);
> + }
> + s++;
> + }
> + return done ? 0 : -ENODEV;
return ret;
> +}
> +
> +static bool of_devlink;
> +core_param(of_devlink, of_devlink, bool, 0);
> +
/**
* of_link_to_suppliers - Add device links to suppliers
* @dev: consumer device
*
* Create device links to all available suppliers of @dev.
* Must NOT stop at the first failed link.
* If some suppliers are not yet available, this function will be
* called again when additional suppliers become available.
*
* Return:
* * 0 if links successfully created for all suppliers
* * an error if one or more suppliers not yet available
*/
> +static int of_link_to_suppliers(struct device *dev)
> +{
> + struct property *p;
> + bool done = true;
remove done
int ret = 0;
> +
> + if (!of_devlink)
> + return 0;
> + if (unlikely(!dev->of_node))
> + return 0;
Check not needed, for_each_property_of_node() will detect !dev->of_node.
> +
> + for_each_property_of_node(dev->of_node, p)
> + if (of_link_property(dev, dev->of_node, p->name))
> + done = false;
ret = -EAGAIN;
> +
> + return done ? 0 : -ENODEV;
return ret;
> +}
> +
> #ifndef CONFIG_PPC
> static const struct of_device_id reserved_mem_matches[] = {
> { .compatible = "qcom,rmtfs-mem" },
> @@ -523,6 +687,7 @@ static int __init of_platform_default_populate_init(void)
> if (!of_have_populated_dt())
> return -ENODEV;
>
> + platform_bus_type.add_links = of_link_to_suppliers;
> /*
> * Handle certain compatibles explicitly, since we don't want to create
> * platform_devices for every node in /reserved-memory with a
> --
> 2.22.0.709.g102302147b-goog
>
>
^ permalink raw reply
* Re: [PATCH v9 0/7] Solve postboot supplier cleanup and optimize probe ordering
From: Frank Rowand @ 2019-08-08 2:13 UTC (permalink / raw)
To: Greg Kroah-Hartman, Saravana Kannan
Cc: Rob Herring, Mark Rutland, Rafael J. Wysocki, devicetree,
linux-kernel, David Collins, kernel-team
In-Reply-To: <20190802063720.GA12789@kroah.com>
Hi Greg, Saravana,
On 8/1/19 11:37 PM, Greg Kroah-Hartman wrote:
> On Thu, Aug 01, 2019 at 12:59:25PM -0700, Frank Rowand wrote:
>> On 8/1/19 12:32 PM, Greg Kroah-Hartman wrote:
>>> On Thu, Aug 01, 2019 at 12:28:13PM -0700, Frank Rowand wrote:
>>>> Hi Greg,
>>>>
>>>> On 7/31/19 11:12 PM, Greg Kroah-Hartman wrote:
>>>>> On Wed, Jul 31, 2019 at 03:17:13PM -0700, Saravana Kannan wrote:
>>>>>> Add device-links to track functional dependencies between devices
>>>>>> after they are created (but before they are probed) by looking at
>>>>>> their common DT bindings like clocks, interconnects, etc.
>>>>>>
>>>>>> Having functional dependencies automatically added before the devices
>>>>>> are probed, provides the following benefits:
>>>>>>
>>>>>> - Optimizes device probe order and avoids the useless work of
>>>>>> attempting probes of devices that will not probe successfully
>>>>>> (because their suppliers aren't present or haven't probed yet).
>>>>>>
>>>>>> For example, in a commonly available mobile SoC, registering just
>>>>>> one consumer device's driver at an initcall level earlier than the
>>>>>> supplier device's driver causes 11 failed probe attempts before the
>>>>>> consumer device probes successfully. This was with a kernel with all
>>>>>> the drivers statically compiled in. This problem gets a lot worse if
>>>>>> all the drivers are loaded as modules without direct symbol
>>>>>> dependencies.
>>>>>>
>>>>>> - Supplier devices like clock providers, interconnect providers, etc
>>>>>> need to keep the resources they provide active and at a particular
>>>>>> state(s) during boot up even if their current set of consumers don't
>>>>>> request the resource to be active. This is because the rest of the
>>>>>> consumers might not have probed yet and turning off the resource
>>>>>> before all the consumers have probed could lead to a hang or
>>>>>> undesired user experience.
>>>>>>
>>>>>> Some frameworks (Eg: regulator) handle this today by turning off
>>>>>> "unused" resources at late_initcall_sync and hoping all the devices
>>>>>> have probed by then. This is not a valid assumption for systems with
>>>>>> loadable modules. Other frameworks (Eg: clock) just don't handle
>>>>>> this due to the lack of a clear signal for when they can turn off
>>>>>> resources. This leads to downstream hacks to handle cases like this
>>>>>> that can easily be solved in the upstream kernel.
>>>>>>
>>>>>> By linking devices before they are probed, we give suppliers a clear
>>>>>> count of the number of dependent consumers. Once all of the
>>>>>> consumers are active, the suppliers can turn off the unused
>>>>>> resources without making assumptions about the number of consumers.
>>>>>>
>>>>>> By default we just add device-links to track "driver presence" (probe
>>>>>> succeeded) of the supplier device. If any other functionality provided
>>>>>> by device-links are needed, it is left to the consumer/supplier
>>>>>> devices to change the link when they probe.
>>>>>
>>>>> All now queued up in my driver-core-testing branch, and if 0-day is
>>>>> happy with this, will move it to my "real" driver-core-next branch in a
>>>>> day or so to get included in linux-next.
>>>>
>>>> I have been slow in getting my review out.
>>>>
>>>> This patch series is not yet ready for sending to Linus, so if putting
>>>> this in linux-next implies that it will be in your next pull request
>>>> to Linus, please do not put it in linux-next.
>>>
>>> It means that it will be in my pull request for 5.4-rc1, many many
>>> waeeks away from now.
>>
>> If you are willing to revert the series before the pull request _if_ I
>> have significant review issues in the next couple of days, then I am happy
>> to see the patches get exposure in linux-next.
>
> If you have significant review issues, yes, I will be glad to revert them.
Just a heads up that I have sent review issues in reply to version 7 of this
patch series.
We'll see what the responses are to my review comments, but I am expecting
the changes are big enough to result in a new version (or a couple more
versions) of the patch series.
No rush to revert version 9 since your 5.4-rc1 pull request is still not
near, and I am glad for whatever exposure these patches are getting in
linux-next.
Thanks,
Frank
>
> thanks,
>
> greg k-h
>
^ permalink raw reply
* Re: [PATCH v2 2/6] thermal: amlogic: Add thermal driver to support G12 SoCs
From: Kevin Hilman @ 2019-08-08 2:59 UTC (permalink / raw)
To: Martin Blumenstingl, guillaume La Roque
Cc: daniel.lezcano, devicetree, linux-amlogic, linux-kernel,
linux-arm-kernel, linux-pm
In-Reply-To: <CAFBinCB3ZBPVEJKV2Rfh_w-zWrhoToYdoYE6Wox+JeB-YH+Khw@mail.gmail.com>
Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
> Hi Guillaume,
>
> On Mon, Aug 5, 2019 at 2:48 PM guillaume La Roque <glaroque@baylibre.com> wrote:
>>
>> Hi Martin,
>>
>> again thanks for your review.
> you're welcome - thank you for working on the driver :-)
>
> [...]
>> > The IP block has more functionality, which may be added to this driver
>> > in the future:
>> > - reading up to 16 stored temperature samples
>>
>> it's not working, you can verify it if you check the regmap define in the driver. in fact temp is only write in one register, it's confirmed by amlogic.
> I missed that - so please skip this part
>
> [...]
>> >> +config AMLOGIC_THERMAL
>> > we typically use "MESON" in the Kconfig symbols:
>> > $ grep -c AMLOGIC .config
>> > 1
>> > $ grep -c MESON .config
>> > 33
>> >
>> > I also wonder if we should add G12 or G12A so we don't conflict with
>> > upcoming thermal sensors with a different design (assuming that this
>> > will be a thing).
>> > for example we already have three different USB2 PHY drivers
>> >
>> > [...]
>>
>> i check with Neil and for new family it's better to use Amlogic instead of meson.
> can you please share the considerations behind this decision?
> if new drivers should use AMLOGIC_* Kconfig symbols instead of MESON_*
> then we all should know about it
>
>> i don't add G12 because we already know it's same sensors for SM1 SoC family [0].
> my idea behind this was to avoid conflicts in the future
> in case of the thermal driver we may be fine with using a generic name
> assuming that Amlogic will not switch to a new IP block in the next
> years
> I'm not saying you have to change the name - I'm bringing this up so
> you can decide for yourself based on examples from the past
>
> here are a few examples:
> - when Kevin upstreamed the MMC driver for GX he decided to use
> MMC_MESON_GX for the Kconfig symbol name. it turns out that this is
> smart because there are at least two other MMC controller IPs on the
> 32-bit SoCs. due to him including GX in the name the drivers are easy
> to differentiate (MMC_MESON_MX_SDIO and MMC_MESON_MX_SDHC being the
> other ones, while the latter is not upstream yet)
> - when Carlo upstreamed the eFuse driver he decided to use MESON_EFUSE
> for the Kconfig symbol name. I found out much later that the 32-bit
> SoCs use a different IP (or at least direct register access instead of
> going through Secure Monitor). the driver for the 32-bit SoCs now uses
> MESON_MX_EFUSE. if you don't know which driver applies where then it's
> easy to mix up MESON_EFUSE and MESON_MX_EFUSE
> - when Jerome upstreamed the ALSA driver for AXG (which is also used
> on G12A and G12B) he decided to use the SND_MESON_AXG_* prefix for the
> Kconfig symbol names. in my opinion this was a good choice because GXM
> and everything earlier (including the 32-bit SoCs) use a different
> audio IP block. we won't have a Kconfig symbol name clash when a
> driver for the "older" SoCs is upstreamed
> - (there are more examples, Meson8b USB PHY driver, Meson8b DWMAC
> glue, ... - just like there's many examples where the IP block is
> mostly compatible with older generations: SAR ADC, RNG, SPI, ...)
While these are all good examples, you can see it can go both ways, so
there's really no way know up front what is the "right" way. We only
know after the fact. Unfortunately, we simply have no visibility into
future chips and where IP blocks may be shared or not (there are other
examples where vendors add a new version of an IP *and* keep the old
version. ;)
Even having worked inside a (different) SoC vendor and having some
knowledge about what IPs are shared, it's difficult to get this right.
> I'm not sure what driver naming rules other mainline SoC teams use
> to me it seems that the rule for Allwinner driver names is to use the
> "code-name of the first SoC the IP block appeared in"
That's a good rule of thumb (and one we generally follow) but I don't
feel it's important enough to enforce strictly either.
Kevin
^ permalink raw reply
* Re: [PATCH 2/6] dt-bindings: arm: amlogic: add bindings for G12B based S922X SoC
From: Kevin Hilman @ 2019-08-08 3:02 UTC (permalink / raw)
To: Rob Herring
Cc: Neil Armstrong, devicetree, linux-amlogic,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-kernel@vger.kernel.org, Christian Hewitt
In-Reply-To: <CAL_JsqL_L2qHe334sB57hR_coRhawKiqXYjKAQDJt_DHfBamBQ@mail.gmail.com>
Rob Herring <robh@kernel.org> writes:
> On Mon, Aug 5, 2019 at 3:46 PM Kevin Hilman <khilman@baylibre.com> wrote:
>>
>> Neil Armstrong <narmstrong@baylibre.com> writes:
>>
>> > Add a specific compatible for the Amlogic G12B family based S922X SoC
>> > to differentiate with the A311D SoC from the same family.
>> >
>> > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> > ---
>> > Documentation/devicetree/bindings/arm/amlogic.yaml | 1 +
>> > 1 file changed, 1 insertion(+)
>> >
>> > diff --git a/Documentation/devicetree/bindings/arm/amlogic.yaml b/Documentation/devicetree/bindings/arm/amlogic.yaml
>> > index 325c6fd3566d..3c3bc806cd23 100644
>> > --- a/Documentation/devicetree/bindings/arm/amlogic.yaml
>> > +++ b/Documentation/devicetree/bindings/arm/amlogic.yaml
>> > @@ -139,6 +139,7 @@ properties:
>> > items:
>> > - enum:
>> > - hardkernel,odroid-n2
>> > + - const: amlogic,s922x
>> > - const: amlogic,g12b
>>
>> nit: in previous binding docs, we were trying to keep these sorted
>> alphabetically. I'll reorder the new "s922x" after "g12b" when
>> applying.
>
> No, this is not documentation ordering. It's the order compatible
> strings must be in.
Ah, thanks for clarifying,
Kevin
^ permalink raw reply
* RE: [PATCH v9 2/6] usb:gadget Separated decoding functions from dwc3 driver.
From: Pawel Laszczak @ 2019-08-08 3:07 UTC (permalink / raw)
To: Felipe Balbi, Roger Quadros, devicetree@vger.kernel.org
Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org,
hdegoede@redhat.com, heikki.krogerus@linux.intel.com,
robh+dt@kernel.org, linux-kernel@vger.kernel.org,
jbergsagel@ti.com, nsekhar@ti.com, nm@ti.com, Suresh Punnoose,
peter.chen@nxp.com, Jayshri Dajiram Pawar, Rahul Kumar
In-Reply-To: <877e7pnula.fsf@gmail.com>
Hi,
>
>Roger Quadros <rogerq@ti.com> writes:
>>>>> +extern const char *usb_decode_ctrl(char *str, size_t size, __u8 bRequestType,
>>>>> + __u8 bRequest, __u16 wValue, __u16 wIndex,
>>>>> + __u16 wLength);
>>>>> +
>>>>
>>>> where's the stub when !TRACING?
>>>
>>> Right, I will add
>>> #ifdef CONFIG_TRACING
>>> .....
>>> #endif
>>
>> Can usb_decode_ctrl() be used even when CONFIG_TRACING is not set?
>> If yes then above #ifdefe is not sufficient.
>>
>> You might need to do something like
>>
>> #if defined(CONFIG_TRACING)
>>
>> extern const char *usb_decode_ctrl(..)
>>
>> #else
>>
>> static inline const char *usb_decode_ctrl(..) {
>> return NULL;
>> }
>>
>> #endif
>
>This is what I mean. They shouldn't be used outside of TRACING, but it's
>far safer to have the stubs.
usb-common-$(CONFIG_TRACING) += debug.o
has been added in Makefile so we don't want to use this if CONFIG_TRACING is not set.
I assume that with this approach this part is correct.
Thanks
Pawell
>
>--
>balbi
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: interrupt-controller: msi: Correct msi-controller@c's reg
From: Bin Meng @ 2019-08-08 3:25 UTC (permalink / raw)
To: Mark Rutland, Rob Herring, devicetree, LKML
In-Reply-To: <CAEUhbmX2LXST-5eDD_UQJP6-XqKPEByVdnQ_KqFM-fR_dH6pyQ@mail.gmail.com>
On Thu, Aug 1, 2019 at 5:53 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Sun, Jul 28, 2019 at 5:30 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > The base address of msi-controller@c should be set to c.
> >
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >
> > Documentation/devicetree/bindings/interrupt-controller/msi.txt | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/msi.txt b/Documentation/devicetree/bindings/interrupt-controller/msi.txt
> > index c60c034..c20b51d 100644
> > --- a/Documentation/devicetree/bindings/interrupt-controller/msi.txt
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/msi.txt
> > @@ -98,7 +98,7 @@ Example
> > };
> >
> > msi_c: msi-controller@c {
> > - reg = <0xb 0xf00>;
> > + reg = <0xc 0xf00>;
> > compatible = "vendor-b,another-controller";
> > msi-controller;
> > /* Each device has some unique ID */
> > --
>
> Ping?
Ping?
^ permalink raw reply
* Re: [PATCH 2/2] dt-bindings: pci: pci-msi: Correct the unit-address of the pci node name
From: Bin Meng @ 2019-08-08 3:26 UTC (permalink / raw)
To: Mark Rutland, Rob Herring, devicetree, LKML
In-Reply-To: <CAEUhbmVjELVPKwW6R+W+V2hQbZ_Zj_5j2ogjnTsuCwnK1pT-og@mail.gmail.com>
On Thu, Aug 1, 2019 at 5:53 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Sun, Jul 28, 2019 at 5:30 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > The unit-address must match the first address specified in the
> > reg property of the node.
> >
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >
> > Documentation/devicetree/bindings/pci/pci-msi.txt | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/pci-msi.txt b/Documentation/devicetree/bindings/pci/pci-msi.txt
> > index 9b3cc81..b73d839 100644
> > --- a/Documentation/devicetree/bindings/pci/pci-msi.txt
> > +++ b/Documentation/devicetree/bindings/pci/pci-msi.txt
> > @@ -201,7 +201,7 @@ Example (5)
> > #msi-cells = <1>;
> > };
> >
> > - pci: pci@c {
> > + pci: pci@f {
> > reg = <0xf 0x1>;
> > compatible = "vendor,pcie-root-complex";
> > device_type = "pci";
> > --
>
> Ping?
Ping?
^ permalink raw reply
* [PATCH v1 0/2] Remove smbus quick cmd and update adapter name
From: Rayagonda Kokatanur @ 2019-08-08 3:37 UTC (permalink / raw)
To: Wolfram Sang, Rob Herring, Mark Rutland
Cc: linux-i2c, devicetree, linux-arm-kernel, linux-kernel,
bcm-kernel-feedback-list, Ray Jui, Rayagonda Kokatanur,
Florian Fainelli
Hi,
This patchset contains following changes:
- Remove SMBUS quick command support
- Update full name of dt node to adapter name
Lori Hikichi (2):
i2c: iproc: Stop advertising support of SMBUS quick cmd
i2c: iproc: Add full name of devicetree node to adapter name
drivers/i2c/busses/i2c-bcm-iproc.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
--
1.9.1
^ permalink raw reply
* [PATCH v1 1/2] i2c: iproc: Stop advertising support of SMBUS quick cmd
From: Rayagonda Kokatanur @ 2019-08-08 3:37 UTC (permalink / raw)
To: Wolfram Sang, Rob Herring, Mark Rutland
Cc: linux-i2c, devicetree, linux-arm-kernel, linux-kernel,
bcm-kernel-feedback-list, Ray Jui, Rayagonda Kokatanur,
Florian Fainelli, Lori Hikichi
In-Reply-To: <1565235473-28461-1-git-send-email-rayagonda.kokatanur@broadcom.com>
From: Lori Hikichi <lori.hikichi@broadcom.com>
The driver does not support the SMBUS Quick command so remove the
flag that indicates that level of support.
By default the i2c_detect tool uses the quick command to try and
detect devices at some bus addresses. If the quick command is used
then we will not detect the device, even though it is present.
Fixes: e6e5dd3566e0 (i2c: iproc: Add Broadcom iProc I2C Driver)
Signed-off-by: Lori Hikichi <lori.hikichi@broadcom.com>
Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
---
drivers/i2c/busses/i2c-bcm-iproc.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index d7fd76b..19ef2b0 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -790,7 +790,10 @@ static int bcm_iproc_i2c_xfer(struct i2c_adapter *adapter,
static uint32_t bcm_iproc_i2c_functionality(struct i2c_adapter *adap)
{
- u32 val = I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+ u32 val;
+
+ /* We do not support the SMBUS Quick command */
+ val = I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
if (adap->algo->reg_slave)
val |= I2C_FUNC_SLAVE;
--
1.9.1
^ permalink raw reply related
* [PATCH v1 2/2] i2c: iproc: Add full name of devicetree node to adapter name
From: Rayagonda Kokatanur @ 2019-08-08 3:37 UTC (permalink / raw)
To: Wolfram Sang, Rob Herring, Mark Rutland
Cc: linux-i2c, devicetree, linux-arm-kernel, linux-kernel,
bcm-kernel-feedback-list, Ray Jui, Rayagonda Kokatanur,
Florian Fainelli, Lori Hikichi
In-Reply-To: <1565235473-28461-1-git-send-email-rayagonda.kokatanur@broadcom.com>
From: Lori Hikichi <lori.hikichi@broadcom.com>
Add the full name of the devicetree node to the adapter name.
Without this change, all adapters have the same name making it difficult
to distinguish between multiple instances.
The most obvious way to see this is to use the utility i2c_detect.
e.g. "i2c-detect -l"
Before
i2c-1 i2c Broadcom iProc I2C adapter I2C adapter
i2c-0 i2c Broadcom iProc I2C adapter I2C adapter
After
i2c-1 i2c Broadcom iProc (i2c@e0000) I2C adapter
i2c-0 i2c Broadcom iProc (i2c@b0000) I2C adapter
Now it is easy to figure out which adapter maps to a which DT node.
Signed-off-by: Lori Hikichi <lori.hikichi@broadcom.com>
Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
---
drivers/i2c/busses/i2c-bcm-iproc.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index 19ef2b0..183b220 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -922,7 +922,9 @@ static int bcm_iproc_i2c_probe(struct platform_device *pdev)
adap = &iproc_i2c->adapter;
i2c_set_adapdata(adap, iproc_i2c);
- strlcpy(adap->name, "Broadcom iProc I2C adapter", sizeof(adap->name));
+ snprintf(adap->name, sizeof(adap->name),
+ "Broadcom iProc (%s)",
+ of_node_full_name(iproc_i2c->device->of_node));
adap->algo = &bcm_iproc_algo;
adap->quirks = &bcm_iproc_i2c_quirks;
adap->dev.parent = &pdev->dev;
--
1.9.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox