* [RFC PATCH 10/11] arm: dts: exynos: Add interconnects to Exynos4412 mixer
From: Artur Świgoń @ 2019-07-23 12:20 UTC (permalink / raw)
To: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel,
linux-pm, dri-devel
Cc: krzk, cw00.choi, myungjoo.ham, inki.dae, sw0312.kim,
georgi.djakov, m.szyprowski
In-Reply-To: <20190723122016.30279-1-a.swigon@partner.samsung.com>
This patch adds an 'interconnects' property to Exynos4412 DTS in order to
declare the interconnect path used by the mixer. Please note that the
'interconnect-names' property is not needed when there is only one path in
'interconnects', in which case calling of_icc_get() with a NULL name simply
returns the right path.
Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
---
arch/arm/boot/dts/exynos4412.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/boot/dts/exynos4412.dtsi b/arch/arm/boot/dts/exynos4412.dtsi
index a70a671acacd..faaec6c40412 100644
--- a/arch/arm/boot/dts/exynos4412.dtsi
+++ b/arch/arm/boot/dts/exynos4412.dtsi
@@ -789,6 +789,7 @@
clock-names = "mixer", "hdmi", "sclk_hdmi", "vp";
clocks = <&clock CLK_MIXER>, <&clock CLK_HDMI>,
<&clock CLK_SCLK_HDMI>, <&clock CLK_VP>;
+ interconnects = <&bus_display &bus_dmc>;
};
&pmu {
--
2.17.1
^ permalink raw reply related
* [RFC PATCH 09/11] devfreq: exynos-bus: Add interconnect functionality to exynos-bus
From: Artur Świgoń @ 2019-07-23 12:20 UTC (permalink / raw)
To: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel,
linux-pm, dri-devel
Cc: krzk, cw00.choi, myungjoo.ham, inki.dae, sw0312.kim,
georgi.djakov, m.szyprowski
In-Reply-To: <20190723122016.30279-1-a.swigon@partner.samsung.com>
This patch adds interconnect functionality to the exynos-bus devfreq
driver.
The SoC topology is a graph (or, more specifically, a tree) and most of its
edges are taken from the devfreq parent-child hierarchy (cf.
Documentation/devicetree/bindings/devfreq/exynos-bus.txt). The previous
patch adds missing edges to the DT (under the name 'parent'). Due to
unspecified relative probing order, -EPROBE_DEFER may be propagated to
guarantee that a child is probed before its parent.
Each bus is now an interconnect provider and an interconnect node as well
(cf. Documentation/interconnect/interconnect.rst), i.e. every bus registers
itself as a node. Node IDs are not hardcoded but rather assigned at
runtime, in probing order (subject to the above-mentioned exception
regarding relative order). This approach allows for using this driver with
various Exynos SoCs.
The devfreq target() callback provided by exynos-bus now selects either the
frequency calculated by the devfreq governor or the frequency requested via
the interconnect API for the given node, whichever is higher.
Please note that it is not an error when CONFIG_INTERCONNECT is 'n', in
which case all interconnect API functions are no-op.
Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
---
drivers/devfreq/exynos-bus.c | 145 +++++++++++++++++++++++++++++++++++
1 file changed, 145 insertions(+)
diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index 412511ca7703..12fb7c84ae50 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -14,6 +14,7 @@
#include <linux/devfreq-event.h>
#include <linux/device.h>
#include <linux/export.h>
+#include <linux/interconnect-provider.h>
#include <linux/module.h>
#include <linux/of.h>
#include <linux/pm_opp.h>
@@ -23,6 +24,8 @@
#define DEFAULT_SATURATION_RATIO 40
#define DEFAULT_VOLTAGE_TOLERANCE 2
+#define icc_units_to_hz(x) ((x) * 1000UL / 8)
+
struct exynos_bus {
struct device *dev;
@@ -31,12 +34,17 @@ struct exynos_bus {
unsigned int edev_count;
struct mutex lock;
+ unsigned long min_freq;
unsigned long curr_freq;
struct regulator *regulator;
struct clk *clk;
unsigned int voltage_tolerance;
unsigned int ratio;
+
+ /* One provider per bus, one node per provider */
+ struct icc_provider provider;
+ struct icc_node *node;
};
/*
@@ -61,6 +69,13 @@ exynos_bus_ops_edev(enable_edev);
exynos_bus_ops_edev(disable_edev);
exynos_bus_ops_edev(set_event);
+static int exynos_bus_next_id(void)
+{
+ static int exynos_bus_node_id;
+
+ return exynos_bus_node_id++;
+}
+
static int exynos_bus_get_event(struct exynos_bus *bus,
struct devfreq_event_data *edata)
{
@@ -98,6 +113,8 @@ static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags)
unsigned long old_freq, new_freq, new_volt, tol;
int ret = 0;
+ *freq = max(*freq, bus->min_freq);
+
/* Get new opp-bus instance according to new bus clock */
new_opp = devfreq_recommended_opp(dev, freq, flags);
if (IS_ERR(new_opp)) {
@@ -208,6 +225,8 @@ static int exynos_bus_passive_target(struct device *dev, unsigned long *freq,
unsigned long old_freq, new_freq;
int ret = 0;
+ *freq = max(*freq, bus->min_freq);
+
/* Get new opp-bus instance according to new bus clock */
new_opp = devfreq_recommended_opp(dev, freq, flags);
if (IS_ERR(new_opp)) {
@@ -251,6 +270,35 @@ static void exynos_bus_passive_exit(struct device *dev)
clk_disable_unprepare(bus->clk);
}
+static int exynos_bus_icc_set(struct icc_node *src, struct icc_node *dst)
+{
+ struct exynos_bus *src_bus = src->data, *dst_bus = dst->data;
+
+ src_bus->min_freq = icc_units_to_hz(src->peak_bw);
+ dst_bus->min_freq = icc_units_to_hz(dst->peak_bw);
+
+ return 0;
+}
+
+static int exynos_bus_icc_aggregate(struct icc_node *node, u32 avg_bw,
+ u32 peak_bw, u32 *agg_avg, u32 *agg_peak)
+{
+ *agg_peak = *agg_avg = peak_bw;
+
+ return 0;
+}
+
+static struct icc_node *exynos_bus_icc_xlate(struct of_phandle_args *spec,
+ void *data)
+{
+ struct exynos_bus *bus = data;
+
+ if (spec->np != bus->dev->of_node)
+ return ERR_PTR(-EINVAL);
+
+ return bus->node;
+}
+
static int exynos_bus_parent_parse_of(struct device_node *np,
struct exynos_bus *bus)
{
@@ -469,6 +517,95 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
return ret;
}
+static int exynos_bus_icc_connect(struct exynos_bus *bus)
+{
+ struct device_node *np = bus->dev->of_node;
+ struct devfreq *parent_devfreq;
+ struct icc_node *parent_node = NULL;
+ struct of_phandle_args args;
+ int ret = 0;
+
+ parent_devfreq = devfreq_get_devfreq_by_phandle(bus->dev, 0);
+ if (!IS_ERR(parent_devfreq)) {
+ struct exynos_bus *parent_bus;
+
+ parent_bus = dev_get_drvdata(parent_devfreq->dev.parent);
+ parent_node = parent_bus->node;
+ } else {
+ /* Look for parent in DT */
+ int num = of_count_phandle_with_args(np, "parent",
+ "#interconnect-cells");
+ if (num != 1)
+ goto out;
+
+ ret = of_parse_phandle_with_args(np, "parent",
+ "#interconnect-cells",
+ 0, &args);
+ if (ret < 0)
+ goto out;
+
+ of_node_put(args.np);
+
+ parent_node = of_icc_get_from_provider(&args);
+ if (IS_ERR(parent_node)) {
+ /* May be -EPROBE_DEFER */
+ ret = PTR_ERR(parent_node);
+ goto out;
+ }
+ }
+
+ ret = icc_link_create(bus->node, parent_node->id);
+
+out:
+ return ret;
+}
+
+static int exynos_bus_icc_init(struct exynos_bus *bus)
+{
+ struct device *dev = bus->dev;
+ struct icc_provider *provider = &bus->provider;
+ struct icc_node *node;
+ int id, ret;
+
+ /* Initialize the interconnect provider */
+ provider->set = exynos_bus_icc_set;
+ provider->aggregate = exynos_bus_icc_aggregate;
+ provider->xlate = exynos_bus_icc_xlate;
+ provider->dev = dev;
+ provider->data = bus;
+
+ ret = icc_provider_add(provider);
+ if (ret < 0)
+ goto out;
+
+ id = exynos_bus_next_id();
+ node = icc_node_create(id);
+ if (IS_ERR(node)) {
+ ret = PTR_ERR(node);
+ goto err_node;
+ }
+
+ bus->node = node;
+ node->name = dev->of_node->name;
+ node->data = bus;
+ icc_node_add(node, provider);
+
+ ret = exynos_bus_icc_connect(bus);
+ if (ret < 0)
+ goto err_connect;
+
+out:
+ return ret;
+
+err_connect:
+ icc_node_del(node);
+ icc_node_destroy(id);
+err_node:
+ icc_provider_del(provider);
+
+ return ret;
+}
+
static int exynos_bus_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -517,6 +654,14 @@ static int exynos_bus_probe(struct platform_device *pdev)
goto err;
}
+ /*
+ * Initialize interconnect provider. A return value of -ENOTSUPP means
+ * that CONFIG_INTERCONNECT is disabled.
+ */
+ ret = exynos_bus_icc_init(bus);
+ if (ret < 0 && ret != -ENOTSUPP)
+ goto err;
+
max_state = bus->devfreq->profile->max_state;
min_freq = (bus->devfreq->profile->freq_table[0] / 1000);
max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000);
--
2.17.1
^ permalink raw reply related
* [RFC PATCH 08/11] arm: dts: exynos: Add parents and #interconnect-cells to Exynos4412
From: Artur Świgoń @ 2019-07-23 12:20 UTC (permalink / raw)
To: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel,
linux-pm, dri-devel
Cc: krzk, cw00.choi, myungjoo.ham, inki.dae, sw0312.kim,
georgi.djakov, m.szyprowski
In-Reply-To: <20190723122016.30279-1-a.swigon@partner.samsung.com>
This patch adds two fields tp the Exynos4412 DTS:
- parent: to declare connections between nodes that are not in a
parent-child relation in devfreq;
- #interconnect-cells: required by the interconnect framework.
Please note that #interconnect-cells is always zero and node IDs are not
hardcoded anywhere.
Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
---
arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 1 +
arch/arm/boot/dts/exynos4412.dtsi | 9 +++++++++
2 files changed, 10 insertions(+)
diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
index ea55f377d17c..bdd61ae86103 100644
--- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
+++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
@@ -106,6 +106,7 @@
&bus_leftbus {
devfreq-events = <&ppmu_leftbus_3>, <&ppmu_rightbus_3>;
vdd-supply = <&buck3_reg>;
+ parent = <&bus_dmc>;
status = "okay";
};
diff --git a/arch/arm/boot/dts/exynos4412.dtsi b/arch/arm/boot/dts/exynos4412.dtsi
index d20db2dfe8e2..a70a671acacd 100644
--- a/arch/arm/boot/dts/exynos4412.dtsi
+++ b/arch/arm/boot/dts/exynos4412.dtsi
@@ -390,6 +390,7 @@
clocks = <&clock CLK_DIV_DMC>;
clock-names = "bus";
operating-points-v2 = <&bus_dmc_opp_table>;
+ #interconnect-cells = <0>;
status = "disabled";
};
@@ -398,6 +399,7 @@
clocks = <&clock CLK_DIV_ACP>;
clock-names = "bus";
operating-points-v2 = <&bus_acp_opp_table>;
+ #interconnect-cells = <0>;
status = "disabled";
};
@@ -406,6 +408,7 @@
clocks = <&clock CLK_DIV_C2C>;
clock-names = "bus";
operating-points-v2 = <&bus_dmc_opp_table>;
+ #interconnect-cells = <0>;
status = "disabled";
};
@@ -459,6 +462,7 @@
clocks = <&clock CLK_DIV_GDL>;
clock-names = "bus";
operating-points-v2 = <&bus_leftbus_opp_table>;
+ #interconnect-cells = <0>;
status = "disabled";
};
@@ -467,6 +471,7 @@
clocks = <&clock CLK_DIV_GDR>;
clock-names = "bus";
operating-points-v2 = <&bus_leftbus_opp_table>;
+ #interconnect-cells = <0>;
status = "disabled";
};
@@ -475,6 +480,7 @@
clocks = <&clock CLK_ACLK160>;
clock-names = "bus";
operating-points-v2 = <&bus_display_opp_table>;
+ #interconnect-cells = <0>;
status = "disabled";
};
@@ -483,6 +489,7 @@
clocks = <&clock CLK_ACLK133>;
clock-names = "bus";
operating-points-v2 = <&bus_fsys_opp_table>;
+ #interconnect-cells = <0>;
status = "disabled";
};
@@ -491,6 +498,7 @@
clocks = <&clock CLK_ACLK100>;
clock-names = "bus";
operating-points-v2 = <&bus_peri_opp_table>;
+ #interconnect-cells = <0>;
status = "disabled";
};
@@ -499,6 +507,7 @@
clocks = <&clock CLK_SCLK_MFC>;
clock-names = "bus";
operating-points-v2 = <&bus_leftbus_opp_table>;
+ #interconnect-cells = <0>;
status = "disabled";
};
--
2.17.1
^ permalink raw reply related
* [RFC PATCH 07/11] icc: Relax condition in apply_constraints()
From: Artur Świgoń @ 2019-07-23 12:20 UTC (permalink / raw)
To: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel,
linux-pm, dri-devel
Cc: krzk, cw00.choi, myungjoo.ham, inki.dae, sw0312.kim,
georgi.djakov, m.szyprowski
In-Reply-To: <20190723122016.30279-1-a.swigon@partner.samsung.com>
The exynos-bus devfreq driver is extended with interconnect functionality
by a subsequent patch. This patch removes a check from the interconnect
framework that prevents interconnect from working on exynos-bus, in which
every bus is a separate interconnect provider.
Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
---
drivers/interconnect/core.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index 2556fd6d1863..bb55565d190c 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -219,11 +219,8 @@ static int apply_constraints(struct icc_path *path)
for (i = 0; i < path->num_nodes; i++) {
next = path->reqs[i].node;
- /*
- * Both endpoints should be valid master-slave pairs of the
- * same interconnect provider that will be configured.
- */
- if (!prev || next->provider != prev->provider) {
+ /* both endpoints should be valid master-slave pairs */
+ if (!prev) {
prev = next;
continue;
}
--
2.17.1
^ permalink raw reply related
* [RFC PATCH 06/11] icc: Relax requirement in of_icc_get_from_provider()
From: Artur Świgoń @ 2019-07-23 12:20 UTC (permalink / raw)
To: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel,
linux-pm, dri-devel
Cc: krzk, cw00.choi, myungjoo.ham, inki.dae, sw0312.kim,
georgi.djakov, m.szyprowski
In-Reply-To: <20190723122016.30279-1-a.swigon@partner.samsung.com>
This patch relaxes the condition in of_icc_get_from_provider() so that it
is no longer required to set #interconnect-cells = <1> in the DT. In case
of the devfreq driver for exynos-bus, #interconnect-cells is always zero.
Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
---
drivers/interconnect/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index d566ae0b66c0..2556fd6d1863 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -279,7 +279,7 @@ struct icc_node *of_icc_get_from_provider(struct of_phandle_args *spec)
struct icc_node *node = ERR_PTR(-EPROBE_DEFER);
struct icc_provider *provider;
- if (!spec || spec->args_count != 1)
+ if (!spec)
return ERR_PTR(-EINVAL);
mutex_lock(&icc_lock);
--
2.17.1
^ permalink raw reply related
* [RFC PATCH 05/11] icc: Export of_icc_get_from_provider()
From: Artur Świgoń @ 2019-07-23 12:20 UTC (permalink / raw)
To: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel,
linux-pm, dri-devel
Cc: krzk, cw00.choi, myungjoo.ham, inki.dae, sw0312.kim,
georgi.djakov, m.szyprowski
In-Reply-To: <20190723122016.30279-1-a.swigon@partner.samsung.com>
This patch makes the above function public (for use in exynos-bus devfreq
driver).
Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
---
drivers/interconnect/core.c | 3 ++-
include/linux/interconnect-provider.h | 6 ++++++
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index 871eb4bc4efc..d566ae0b66c0 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -274,7 +274,7 @@ EXPORT_SYMBOL_GPL(of_icc_xlate_onecell);
* Returns a valid pointer to struct icc_node on success or ERR_PTR()
* on failure.
*/
-static struct icc_node *of_icc_get_from_provider(struct of_phandle_args *spec)
+struct icc_node *of_icc_get_from_provider(struct of_phandle_args *spec)
{
struct icc_node *node = ERR_PTR(-EPROBE_DEFER);
struct icc_provider *provider;
@@ -293,6 +293,7 @@ static struct icc_node *of_icc_get_from_provider(struct of_phandle_args *spec)
return node;
}
+EXPORT_SYMBOL_GPL(of_icc_get_from_provider);
/**
* of_icc_get() - get a path handle from a DT node based on name
diff --git a/include/linux/interconnect-provider.h b/include/linux/interconnect-provider.h
index 63caccadc2db..9ecfc518b952 100644
--- a/include/linux/interconnect-provider.h
+++ b/include/linux/interconnect-provider.h
@@ -97,6 +97,7 @@ void icc_node_add(struct icc_node *node, struct icc_provider *provider);
void icc_node_del(struct icc_node *node);
int icc_provider_add(struct icc_provider *provider);
int icc_provider_del(struct icc_provider *provider);
+struct icc_node *of_icc_get_from_provider(struct of_phandle_args *spec);
#else
@@ -137,6 +138,11 @@ static inline int icc_provider_del(struct icc_provider *provider)
return -ENOTSUPP;
}
+struct icc_node *of_icc_get_from_provider(struct of_phandle_args *spec)
+{
+ return ERR_PTR(-ENOTSUPP);
+}
+
#endif /* CONFIG_INTERCONNECT */
#endif /* __LINUX_INTERCONNECT_PROVIDER_H */
--
2.17.1
^ permalink raw reply related
* [RFC PATCH 04/11] devfreq: exynos-bus: Clean up code
From: Artur Świgoń @ 2019-07-23 12:20 UTC (permalink / raw)
To: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel,
linux-pm, dri-devel
Cc: krzk, cw00.choi, myungjoo.ham, inki.dae, sw0312.kim,
georgi.djakov, m.szyprowski
In-Reply-To: <20190723122016.30279-1-a.swigon@partner.samsung.com>
This patch adds minor improvements to the exynos-bus driver.
Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
---
drivers/devfreq/exynos-bus.c | 49 ++++++++++++++++--------------------
1 file changed, 22 insertions(+), 27 deletions(-)
diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index 4bb83b945bf7..412511ca7703 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -15,11 +15,10 @@
#include <linux/device.h>
#include <linux/export.h>
#include <linux/module.h>
-#include <linux/of_device.h>
+#include <linux/of.h>
#include <linux/pm_opp.h>
#include <linux/platform_device.h>
#include <linux/regulator/consumer.h>
-#include <linux/slab.h>
#define DEFAULT_SATURATION_RATIO 40
#define DEFAULT_VOLTAGE_TOLERANCE 2
@@ -256,7 +255,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
struct exynos_bus *bus)
{
struct device *dev = bus->dev;
- int i, ret, count, size;
+ int i, ret, count;
/* Get the regulator to provide each bus with the power */
bus->regulator = devm_regulator_get(dev, "vdd");
@@ -283,8 +282,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
}
bus->edev_count = count;
- size = sizeof(*bus->edev) * count;
- bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
+ bus->edev = devm_kcalloc(dev, count, sizeof(*bus->edev), GFP_KERNEL);
if (!bus->edev) {
ret = -ENOMEM;
goto err_regulator;
@@ -388,7 +386,7 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
ondemand_data = devm_kzalloc(dev, sizeof(*ondemand_data), GFP_KERNEL);
if (!ondemand_data) {
ret = -ENOMEM;
- goto err;
+ goto out;
}
ondemand_data->upthreshold = 40;
ondemand_data->downdifferential = 5;
@@ -400,14 +398,14 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
if (IS_ERR(bus->devfreq)) {
dev_err(dev, "failed to add devfreq device\n");
ret = PTR_ERR(bus->devfreq);
- goto err;
+ goto out;
}
/* Register opp_notifier to catch the change of OPP */
ret = devm_devfreq_register_opp_notifier(dev, bus->devfreq);
if (ret < 0) {
dev_err(dev, "failed to register opp notifier\n");
- goto err;
+ goto out;
}
/*
@@ -417,16 +415,16 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
ret = exynos_bus_enable_edev(bus);
if (ret < 0) {
dev_err(dev, "failed to enable devfreq-event devices\n");
- goto err;
+ goto out;
}
ret = exynos_bus_set_event(bus);
if (ret < 0) {
dev_err(dev, "failed to set event to devfreq-event devices\n");
- goto err;
+ goto out;
}
-err:
+out:
return ret;
}
@@ -446,27 +444,28 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
parent_devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
if (IS_ERR(parent_devfreq)) {
ret = -EPROBE_DEFER;
- goto err;
+ goto out;
}
passive_data = devm_kzalloc(dev, sizeof(*passive_data), GFP_KERNEL);
if (!passive_data) {
ret = -ENOMEM;
- goto err;
+ goto out;
}
passive_data->parent = parent_devfreq;
/* Add devfreq device for exynos bus with passive governor */
- bus->devfreq = devm_devfreq_add_device(dev, profile, DEVFREQ_GOV_PASSIVE,
+ bus->devfreq = devm_devfreq_add_device(dev, profile,
+ DEVFREQ_GOV_PASSIVE,
passive_data);
if (IS_ERR(bus->devfreq)) {
dev_err(dev,
"failed to add devfreq dev with passive governor\n");
ret = PTR_ERR(bus->devfreq);
- goto err;
+ goto out;
}
-err:
+out:
return ret;
}
@@ -484,11 +483,11 @@ static int exynos_bus_probe(struct platform_device *pdev)
return -EINVAL;
}
- bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
+ bus = devm_kzalloc(dev, sizeof(*bus), GFP_KERNEL);
if (!bus)
return -ENOMEM;
mutex_init(&bus->lock);
- bus->dev = &pdev->dev;
+ bus->dev = dev;
platform_set_drvdata(pdev, bus);
/* Parse the device-tree to get the resource information */
@@ -502,7 +501,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
goto err;
}
- node = of_parse_phandle(dev->of_node, "devfreq", 0);
+ node = of_parse_phandle(np, "devfreq", 0);
if (node) {
of_node_put(node);
ret = exynos_bus_profile_init_passive(bus, profile);
@@ -547,12 +546,10 @@ static int exynos_bus_resume(struct device *dev)
int ret;
ret = exynos_bus_enable_edev(bus);
- if (ret < 0) {
+ if (ret < 0)
dev_err(dev, "failed to enable the devfreq-event devices\n");
- return ret;
- }
- return 0;
+ return ret;
}
static int exynos_bus_suspend(struct device *dev)
@@ -561,12 +558,10 @@ static int exynos_bus_suspend(struct device *dev)
int ret;
ret = exynos_bus_disable_edev(bus);
- if (ret < 0) {
+ if (ret < 0)
dev_err(dev, "failed to disable the devfreq-event devices\n");
- return ret;
- }
- return 0;
+ return ret;
}
#endif
--
2.17.1
^ permalink raw reply related
* [RFC PATCH 03/11] devfreq: exynos-bus: Change goto-based logic to if-else logic
From: Artur Świgoń @ 2019-07-23 12:20 UTC (permalink / raw)
To: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel,
linux-pm, dri-devel
Cc: sw0312.kim, krzk, inki.dae, cw00.choi, myungjoo.ham,
georgi.djakov, m.szyprowski
In-Reply-To: <20190723122016.30279-1-a.swigon@partner.samsung.com>
This patch improves code readability by changing the following construct:
> if (cond)
> goto passive;
> foo();
> goto out;
>passive:
> bar();
>out:
into this:
> if (cond)
> bar();
> else
> foo();
Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
---
drivers/devfreq/exynos-bus.c | 24 +++++++++---------------
1 file changed, 9 insertions(+), 15 deletions(-)
diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index cf6f6cbd0f55..4bb83b945bf7 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -505,25 +505,19 @@ static int exynos_bus_probe(struct platform_device *pdev)
node = of_parse_phandle(dev->of_node, "devfreq", 0);
if (node) {
of_node_put(node);
- goto passive;
+ ret = exynos_bus_profile_init_passive(bus, profile);
+ if (ret < 0)
+ goto err;
} else {
ret = exynos_bus_parent_parse_of(np, bus);
+ if (ret < 0)
+ goto err;
+
+ ret = exynos_bus_profile_init(bus, profile);
+ if (ret < 0)
+ goto err;
}
- if (ret < 0)
- goto err;
-
- ret = exynos_bus_profile_init(bus, profile);
- if (ret < 0)
- goto err;
-
- goto out;
-passive:
- ret = exynos_bus_profile_init_passive(bus, profile);
- if (ret < 0)
- goto err;
-
-out:
max_state = bus->devfreq->profile->max_state;
min_freq = (bus->devfreq->profile->freq_table[0] / 1000);
max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000);
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [RFC PATCH 02/11] devfreq: exynos-bus: Extract exynos_bus_profile_init_passive()
From: Artur Świgoń @ 2019-07-23 12:20 UTC (permalink / raw)
To: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel,
linux-pm, dri-devel
Cc: sw0312.kim, krzk, inki.dae, cw00.choi, myungjoo.ham,
georgi.djakov, m.szyprowski
In-Reply-To: <20190723122016.30279-1-a.swigon@partner.samsung.com>
This patch adds a new static function, exynos_bus_profile_init_passive(),
extracted from exynos_bus_probe().
Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
---
drivers/devfreq/exynos-bus.c | 70 +++++++++++++++++++++---------------
1 file changed, 42 insertions(+), 28 deletions(-)
diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index d8f1efaf2d49..cf6f6cbd0f55 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -430,13 +430,51 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
return ret;
}
+static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
+ struct devfreq_dev_profile *profile)
+{
+ struct device *dev = bus->dev;
+ struct devfreq *parent_devfreq;
+ struct devfreq_passive_data *passive_data;
+ int ret = 0;
+
+ /* Initialize the struct profile and governor data for passive device */
+ profile->target = exynos_bus_passive_target;
+ profile->exit = exynos_bus_passive_exit;
+
+ /* Get the instance of parent devfreq device */
+ parent_devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
+ if (IS_ERR(parent_devfreq)) {
+ ret = -EPROBE_DEFER;
+ goto err;
+ }
+
+ passive_data = devm_kzalloc(dev, sizeof(*passive_data), GFP_KERNEL);
+ if (!passive_data) {
+ ret = -ENOMEM;
+ goto err;
+ }
+ passive_data->parent = parent_devfreq;
+
+ /* Add devfreq device for exynos bus with passive governor */
+ bus->devfreq = devm_devfreq_add_device(dev, profile, DEVFREQ_GOV_PASSIVE,
+ passive_data);
+ if (IS_ERR(bus->devfreq)) {
+ dev_err(dev,
+ "failed to add devfreq dev with passive governor\n");
+ ret = PTR_ERR(bus->devfreq);
+ goto err;
+ }
+
+err:
+ return ret;
+}
+
static int exynos_bus_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct device_node *np = dev->of_node, *node;
struct devfreq_dev_profile *profile;
- struct devfreq_passive_data *passive_data;
- struct devfreq *parent_devfreq;
struct exynos_bus *bus;
int ret, max_state;
unsigned long min_freq, max_freq;
@@ -481,33 +519,9 @@ 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->exit = exynos_bus_passive_exit;
-
- /* Get the instance of parent devfreq device */
- parent_devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
- if (IS_ERR(parent_devfreq)) {
- ret = -EPROBE_DEFER;
+ ret = exynos_bus_profile_init_passive(bus, profile);
+ if (ret < 0)
goto err;
- }
-
- passive_data = devm_kzalloc(dev, sizeof(*passive_data), GFP_KERNEL);
- if (!passive_data) {
- ret = -ENOMEM;
- goto err;
- }
- passive_data->parent = parent_devfreq;
-
- /* Add devfreq device for exynos bus with passive governor */
- bus->devfreq = devm_devfreq_add_device(dev, profile, DEVFREQ_GOV_PASSIVE,
- passive_data);
- if (IS_ERR(bus->devfreq)) {
- dev_err(dev,
- "failed to add devfreq dev with passive governor\n");
- ret = PTR_ERR(bus->devfreq);
- goto err;
- }
out:
max_state = bus->devfreq->profile->max_state;
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [RFC PATCH 01/11] devfreq: exynos-bus: Extract exynos_bus_profile_init()
From: Artur Świgoń @ 2019-07-23 12:20 UTC (permalink / raw)
To: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel,
linux-pm, dri-devel
Cc: krzk, cw00.choi, myungjoo.ham, inki.dae, sw0312.kim,
georgi.djakov, m.szyprowski
In-Reply-To: <20190723122016.30279-1-a.swigon@partner.samsung.com>
This patch adds a new static function, exynos_bus_profile_init(), extracted
from exynos_bus_probe().
Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
---
drivers/devfreq/exynos-bus.c | 106 ++++++++++++++++++++---------------
1 file changed, 60 insertions(+), 46 deletions(-)
diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index d9f377912c10..d8f1efaf2d49 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -372,12 +372,69 @@ static int exynos_bus_parse_of(struct device_node *np,
return ret;
}
+static int exynos_bus_profile_init(struct exynos_bus *bus,
+ struct devfreq_dev_profile *profile)
+{
+ struct device *dev = bus->dev;
+ struct devfreq_simple_ondemand_data *ondemand_data;
+ int ret;
+
+ /* Initialize the struct profile and governor data for parent device */
+ profile->polling_ms = 50;
+ profile->target = exynos_bus_target;
+ profile->get_dev_status = exynos_bus_get_dev_status;
+ profile->exit = exynos_bus_exit;
+
+ ondemand_data = devm_kzalloc(dev, sizeof(*ondemand_data), GFP_KERNEL);
+ if (!ondemand_data) {
+ ret = -ENOMEM;
+ goto err;
+ }
+ ondemand_data->upthreshold = 40;
+ ondemand_data->downdifferential = 5;
+
+ /* Add devfreq device to monitor and handle the exynos bus */
+ bus->devfreq = devm_devfreq_add_device(dev, profile,
+ DEVFREQ_GOV_SIMPLE_ONDEMAND,
+ ondemand_data);
+ if (IS_ERR(bus->devfreq)) {
+ dev_err(dev, "failed to add devfreq device\n");
+ ret = PTR_ERR(bus->devfreq);
+ goto err;
+ }
+
+ /* Register opp_notifier to catch the change of OPP */
+ ret = devm_devfreq_register_opp_notifier(dev, bus->devfreq);
+ if (ret < 0) {
+ dev_err(dev, "failed to register opp notifier\n");
+ goto err;
+ }
+
+ /*
+ * Enable devfreq-event to get raw data which is used to determine
+ * current bus load.
+ */
+ ret = exynos_bus_enable_edev(bus);
+ if (ret < 0) {
+ dev_err(dev, "failed to enable devfreq-event devices\n");
+ goto err;
+ }
+
+ ret = exynos_bus_set_event(bus);
+ if (ret < 0) {
+ dev_err(dev, "failed to set event to devfreq-event devices\n");
+ goto err;
+ }
+
+err:
+ return ret;
+}
+
static int exynos_bus_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct device_node *np = dev->of_node, *node;
struct devfreq_dev_profile *profile;
- struct devfreq_simple_ondemand_data *ondemand_data;
struct devfreq_passive_data *passive_data;
struct devfreq *parent_devfreq;
struct exynos_bus *bus;
@@ -418,52 +475,9 @@ static int exynos_bus_probe(struct platform_device *pdev)
if (ret < 0)
goto err;
- /* Initialize the struct profile and governor data for parent device */
- profile->polling_ms = 50;
- profile->target = exynos_bus_target;
- profile->get_dev_status = exynos_bus_get_dev_status;
- profile->exit = exynos_bus_exit;
-
- ondemand_data = devm_kzalloc(dev, sizeof(*ondemand_data), GFP_KERNEL);
- if (!ondemand_data) {
- ret = -ENOMEM;
+ ret = exynos_bus_profile_init(bus, profile);
+ if (ret < 0)
goto err;
- }
- ondemand_data->upthreshold = 40;
- ondemand_data->downdifferential = 5;
-
- /* Add devfreq device to monitor and handle the exynos bus */
- bus->devfreq = devm_devfreq_add_device(dev, profile,
- DEVFREQ_GOV_SIMPLE_ONDEMAND,
- ondemand_data);
- if (IS_ERR(bus->devfreq)) {
- dev_err(dev, "failed to add devfreq device\n");
- ret = PTR_ERR(bus->devfreq);
- goto err;
- }
-
- /* Register opp_notifier to catch the change of OPP */
- ret = devm_devfreq_register_opp_notifier(dev, bus->devfreq);
- if (ret < 0) {
- dev_err(dev, "failed to register opp notifier\n");
- goto err;
- }
-
- /*
- * Enable devfreq-event to get raw data which is used to determine
- * current bus load.
- */
- ret = exynos_bus_enable_edev(bus);
- if (ret < 0) {
- dev_err(dev, "failed to enable devfreq-event devices\n");
- goto err;
- }
-
- ret = exynos_bus_set_event(bus);
- if (ret < 0) {
- dev_err(dev, "failed to set event to devfreq-event devices\n");
- goto err;
- }
goto out;
passive:
--
2.17.1
^ permalink raw reply related
* [RFC PATCH 00/11] Simple QoS for exynos-bus driver using interconnect
From: Artur Świgoń @ 2019-07-23 12:20 UTC (permalink / raw)
To: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel,
linux-pm, dri-devel
Cc: krzk, cw00.choi, myungjoo.ham, inki.dae, sw0312.kim,
georgi.djakov, m.szyprowski
In-Reply-To: <CGME20190723122022eucas1p2f568f74f981f9de9012eb693c3b446d5@eucas1p2.samsung.com>
The following patchset adds interconnect[1][2] framework support to the
exynos-bus devfreq driver. Extending the devfreq driver with interconnect
capabilities started as a response to the issue referenced in [3]. The
patches can be subdivided into four logical groups:
(a) Refactoring the existing devfreq driver in order to improve readability
and accommodate for adding new code (patches 01--04/11).
(b) Tweaking the interconnect framework to support the exynos-bus use case
(patches 05--07/11). Exporting of_icc_get_from_provider() allows us to
avoid hardcoding every single graph edge in the DT or driver source, and
relaxing the requirement contained in that function removes the need to
provide dummy node IDs in the DT. Adjusting the logic in
apply_constraints() (drivers/interconnect/core.c) accounts for the fact
that every bus is a separate entity and therefore a separate interconnect
provider, albeit constituting a part of a larger hierarchy.
(c) Implementing interconnect providers in the exynos-bus devfreq driver
and adding required DT properties for one selected platform, namely
Exynos4412 (patches 08--09/11). Due to the fact that this aims to be a
generic driver for various Exynos SoCs, node IDs are generated dynamically
rather than hardcoded. This has been determined to be a simpler approach,
but depends on changes described in (b).
(d) Implementing a sample interconnect consumer for exynos-mixer targeted
at the issue referenced in [3], again with DT info only for Exynos4412
(patches 10--11/11).
Integration of devfreq and interconnect functionalities comes down to one
extra line in the devfreq target() callback, which selects either the
frequency calculated by the devfreq governor, or the one requested with the
interconnect API, whichever is higher. All new code works equally well when
CONFIG_INTERCONNECT is 'n' (as in exynos_defconfig) in which case all
interconnect API functions are no-ops.
---
Artur Świgoń
Samsung R&D Institute Poland
Samsung Electronics
---
References:
[1] Documentation/interconnect/interconnect.rst
[2] Documentation/devicetree/bindings/interconnect/interconnect.txt
[3] https://patchwork.kernel.org/patch/10861757
Artur Świgoń (10):
devfreq: exynos-bus: Extract exynos_bus_profile_init()
devfreq: exynos-bus: Extract exynos_bus_profile_init_passive()
devfreq: exynos-bus: Change goto-based logic to if-else logic
devfreq: exynos-bus: Clean up code
icc: Export of_icc_get_from_provider()
icc: Relax requirement in of_icc_get_from_provider()
icc: Relax condition in apply_constraints()
arm: dts: exynos: Add parents and #interconnect-cells to Exynos4412
devfreq: exynos-bus: Add interconnect functionality to exynos-bus
arm: dts: exynos: Add interconnects to Exynos4412 mixer
Marek Szyprowski (1):
drm: exynos: mixer: Add interconnect support
.../boot/dts/exynos4412-odroid-common.dtsi | 1 +
arch/arm/boot/dts/exynos4412.dtsi | 10 +
drivers/devfreq/exynos-bus.c | 296 ++++++++++++++----
drivers/gpu/drm/exynos/exynos_mixer.c | 68 +++-
drivers/interconnect/core.c | 12 +-
include/linux/interconnect-provider.h | 6 +
6 files changed, 314 insertions(+), 79 deletions(-)
--
2.17.1
^ permalink raw reply
* [PATCH] ARM: dts: da850-evm: Use generic jedec, spi-nor for flash
From: Adam Ford @ 2019-07-23 12:10 UTC (permalink / raw)
To: linux-arm-kernel
Cc: adam.ford, Adam Ford, Sekhar Nori, Bartosz Golaszewski,
Rob Herring, Mark Rutland, devicetree, linux-kernel
Logic PD re-spun the L138 and AM1808 SOM's with larger flash.
The m25p80 driver has a generic 'jedec,spi-nor' compatible option
which is requests to use whenever possible since it will read the
JEDEC READ ID opcode.
Signed-off-by: Adam Ford <aford173@gmail.com>
diff --git a/arch/arm/boot/dts/da850-evm.dts b/arch/arm/boot/dts/da850-evm.dts
index 5b2b1ed04d51..f2e7609e5346 100644
--- a/arch/arm/boot/dts/da850-evm.dts
+++ b/arch/arm/boot/dts/da850-evm.dts
@@ -281,7 +281,7 @@
flash: m25p80@0 {
#address-cells = <1>;
#size-cells = <1>;
- compatible = "m25p64";
+ compatible = "jedec,spi-nor";
spi-max-frequency = <30000000>;
m25p,fast-read;
reg = <0>;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v1 27/50] ARM: dts: exynos: align bus_wcore OPPs in Exynos5420
From: Krzysztof Kozlowski @ 2019-07-23 12:08 UTC (permalink / raw)
To: Lukasz Luba
Cc: devicetree, linux-kernel, linux-arm-kernel,
linux-samsung-soc@vger.kernel.org, linux-clk, mturquette, sboyd,
Bartłomiej Żołnierkiewicz, kgene, mark.rutland,
robh+dt, Chanwoo Choi, kyungmin.park, Andrzej Hajda,
Marek Szyprowski, s.nawrocki, myungjoo.ham
In-Reply-To: <15310bd0-0fd4-8d62-82a9-7a9df6a8ced3@partner.samsung.com>
On Wed, 17 Jul 2019 at 18:58, Lukasz Luba <l.luba@partner.samsung.com> wrote:
>
> Hi Krzysztof,
>
> On 7/17/19 12:15 PM, Krzysztof Kozlowski wrote:
> > On Mon, 15 Jul 2019 at 14:44, Lukasz Luba <l.luba@partner.samsung.com> wrote:
> >>
> >> This is the most important bus in the Exynos5x SoC. The whole communication
> >> inside SoC does through that bus (apart from direct requests from CCI to
> >> DRAM controller). It is also modeled as a master bus in devfreq framework.
> >> It is also the only one OPP table throughout other buses which has voltage
> >> values. The devfreq software controls the speed of that bus and other
> >> buses. The other buses follows the rate of the master. There is only one
> >> regulator. The old lowest OPP had pair 925mV, 84MHz which is enough for
> >
> > s/lowest/slowest/
> please see below
> >
> >> this frequency. However, due to the fact that the other buses follows the
> >> WCORE bus by taking the OPP from their table with the same id, e.g. opp02,
> >> the children frequency should be stable with the set voltage.
> >> It could cause random faults very hard to debug.
> >> Thus, the patch removes the lowest OPP to make other buses' lowest OPPs
> >
> > s/lowest/slowest/
> Actually, I have double checked that, because we always used this
> terminology: low OPP, high OPP, lower OPPs, higher OPPs. I can change
> it here for you, but I think this is not something that people are used
> to. Please check EAS pdf documentation or this file:
> https://www.kernel.org/doc/Documentation/scheduler/sched-energy.txt
> i.e. "running at a lower OPP" or "high OPPs", "lowest OPPs".
Hmm, indeed, you're right. Don't change it then.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v1 37/50] ARM: dts: exynos: change parent and rate of bus_fsys in Exynos5422
From: Krzysztof Kozlowski @ 2019-07-23 12:06 UTC (permalink / raw)
To: Lukasz Luba
Cc: devicetree, linux-kernel, linux-arm-kernel,
linux-samsung-soc@vger.kernel.org, linux-clk, mturquette, sboyd,
Bartłomiej Żołnierkiewicz, kgene, mark.rutland,
robh+dt, Chanwoo Choi, kyungmin.park, Andrzej Hajda,
Marek Szyprowski, s.nawrocki, myungjoo.ham
In-Reply-To: <7ad899c5-347d-546e-a2e9-d96f0203210c@partner.samsung.com>
On Wed, 17 Jul 2019 at 14:56, Lukasz Luba <l.luba@partner.samsung.com> wrote:
>
>
> On 7/17/19 1:11 PM, Krzysztof Kozlowski wrote:
> > On Wed, 17 Jul 2019 at 13:06, Lukasz Luba <l.luba@partner.samsung.com> wrote:
> >>
> >>
> >>
> >> On 7/17/19 12:45 PM, Krzysztof Kozlowski wrote:
> >>> On Wed, 17 Jul 2019 at 12:39, Lukasz Luba <l.luba@partner.samsung.com> wrote:
> >>>>>>
> >>>>>> &bus_fsys {
> >>>>>> devfreq = <&bus_wcore>;
> >>>>>> + assigned-clocks = <&clock CLK_MOUT_ACLK200_FSYS>,
> >>>>>> + <&clock CLK_DOUT_ACLK200_FSYS>,
> >>>>>> + <&clock CLK_FOUT_DPLL>;
> >>>>>> + assigned-clock-parents = <&clock CLK_MOUT_SCLK_DPLL>;
> >>>>>> + assigned-clock-rates = <0>, <240000000>,<1200000000>;
> >>>>>
> >>>>> Here and in all other patches:
> >>>>> I am not entirely sure that this should be here. It looks like
> >>>>> property of the SoC. Do we expect that buses will be configured to
> >>>>> different clock rates between different boards?
> This is the board file for Exynos5420/5422/5800 which enables buses.
> Thus, I have change them here. Patch 49/50 adds these buses to
> Exynos5800 (Peach Pi). In Exynos5420 there is no clock tree for
> bus_isp266. The parents for different devices could be also different.
> It is because i.e. in 5420 there is 2 bit in the WCORE 1st mux while in
> 5422 there is 3 bits (6 parents possible).
> That's why I have picked exynos5422-odroid-core.dtsi to reference
> the bus devices and pinned them into proper parent and changed rate.
> When you check patch 49/50 for 5800 not all the parents are the same.
>
> (1) I could create a dedicated files like: exynos5422-bus.dtsi,
> exynos5420-bus.dtsi, exynos5800-bus.dtsi which would include some
> base file with the basic &bus_X and set the right parent, rate.
> Then these files would be included into proper board file like:
> exynos5800-peach-pi.dts.
> Is this something that you would like to see?
I see now. Are there any differences in all these properties between
Peach Pi and Odroids? Both of them are using exynos5800.dtsi so that
could be a place for all clock assignments.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v4 0/5] soc: ti: k3: Allow for exclusive and shared device requests
From: Nishanth Menon @ 2019-07-23 11:42 UTC (permalink / raw)
To: Lokesh Vutla
Cc: Device Tree Mailing List, Sekhar Nori, Tero Kristo, Rob Herring,
Santosh Shilimkar, Linux ARM Mailing List
In-Reply-To: <20190722050757.29893-1-lokeshvutla@ti.com>
On 10:37-20190722, Lokesh Vutla wrote:
> Sysfw provides an option for requesting exclusive access for a
> device using the flags MSG_FLAG_DEVICE_EXCLUSIVE. If this flag is
> not used, the device is meant to be shared across hosts. Once a device
> is requested from a host with this flag set, any request to this
> device from a different host will be nacked by sysfw.
>
> Current tisci firmware and pm drivers always requests for device with
> exclusive permissions set. But this is not be true for certain devices
> that are expcted to be shared across different host contexts.
> So add support for getting the shared or exclusive permissions from DT
> and request firmware accordingly.
>
> Changes since v3: https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=103447
> - Rebased on top of v5.3-rc1
> - Updated power-domain cells for j721e.
> - Mark the console uart as shared in am65x-base-board
> - Added Reviewed-by from Rob
[...]
Looks good to me.
Reviewed-by: Nishanth Menon <nm@ti.com>
--
Regards,
Nishanth Menon
^ permalink raw reply
* Re: [PATCH 3/4] arm64: dts: ti: k3-j721e-main: Add mailbox cluster nodes
From: Nishanth Menon @ 2019-07-23 11:37 UTC (permalink / raw)
To: Suman Anna; +Cc: Tero Kristo, devicetree, linux-arm-kernel
In-Reply-To: <20190722202024.14867-4-s-anna@ti.com>
On 15:20-20190722, Suman Anna wrote:
> The J721E Main NavSS block contains a Mailbox IP instance with
> multiple clusters. Each cluster is equivalent to an Mailbox IP
> instance on OMAP platforms.
>
> Add all the Mailbox clusters as their own nodes under the MAIN
> NavSS cbass_main_navss interconnect node instead of creating an
> almost empty parent node for the new K3 mailbox IP and the clusters
> as its child nodes. All these nodes are marked as disabled, and
> they need to be enabled along with the appropriate child nodes
> on a need basis.
>
> NOTE:
> The NavSS only has a limited number of interrupts, so all the
> interrupts generated by a Mailbox IP are not added by default.
> Only the needed interrupts that are targeted towards the A72
> GIC will need to be be added later on when some sub-mailbox
> child nodes are added.
>
> Signed-off-by: Suman Anna <s-anna@ti.com>
> ---
> arch/arm64/boot/dts/ti/k3-j721e-main.dtsi | 108 ++++++++++++++++++++++
> 1 file changed, 108 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi b/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
> index a2e031f7d88e..319d423b3440 100644
> --- a/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
> @@ -95,6 +95,114 @@
> reg = <0x00 0x30e00000 0x00 0x1000>;
> #hwlock-cells = <1>;
> };
> +
> + mailbox0_cluster0: mailbox@31f80000 {
> + compatible = "ti,am654-mailbox";
> + reg = <0x00 0x31f80000 0x00 0x200>;
> + #mbox-cells = <1>;
> + ti,mbox-num-users = <4>;
> + ti,mbox-num-fifos = <16>;
> + status = "disabled";
Please drop disabled.
--
Regards,
Nishanth Menon
^ permalink raw reply
* Re: [PATCH 2/4] arm64: dts: ti: k3-am65-main: Add IPC sub-mailbox nodes for R5Fs
From: Nishanth Menon @ 2019-07-23 11:37 UTC (permalink / raw)
To: Suman Anna; +Cc: Tero Kristo, devicetree, linux-arm-kernel
In-Reply-To: <20190722202024.14867-3-s-anna@ti.com>
On 15:20-20190722, Suman Anna wrote:
> Add the sub-mailbox nodes that are used to communicate between
> MPU and the two R5F remote processors present in the MCU domain.
> The parent mailbox cluster nodes are enabled and the interrupts
> associated with the Mailbox Cluster User interrupt used by the
> sub-mailbox nodes are also added. The GIC_SPI interrupt to be
> used is dynamically allocated and managed by the System Firmware
> through the ti-sci-intr irqchip driver.
>
> The sub-mailbox nodes utilize the System Mailbox clusters 1 and 2.
> These sub-mailbox nodes are added to match the hard-coded mailbox
> configuration used within the TI RTOS IPC software packages. The
> Cortex R5F processor sub-system is assumed to be running in Split
> mode, so a sub-mailbox node is used by each of the R5F cores. Only
> the sub-mailbox node from cluster 0 is used in case of Lockstep
> mode.
>
> Signed-off-by: Suman Anna <s-anna@ti.com>
> ---
> arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
> index 0b3ea2a871ee..317563c995b1 100644
> --- a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
> @@ -426,7 +426,13 @@
> #mbox-cells = <1>;
> ti,mbox-num-users = <4>;
> ti,mbox-num-fifos = <16>;
> - status = "disabled";
> + interrupt-parent = <&intr_main_navss>;
> + interrupts = <164 0>;
> +
> + mbox_mcu_r5fss0_core0: mbox-mcu-r5fss0-core0 {
> + ti,mbox-tx = <1 0 0>;
> + ti,mbox-rx = <0 0 0>;
> + };
Should'nt this rather be a board specific node? This is completely
dependent on the pre-defined communication scheme with R5F firmware?
> };
>
> mailbox0_cluster1: mailbox@31f81000 {
> @@ -435,7 +441,13 @@
> #mbox-cells = <1>;
> ti,mbox-num-users = <4>;
> ti,mbox-num-fifos = <16>;
> - status = "disabled";
> + interrupt-parent = <&intr_main_navss>;
> + interrupts = <165 0>;
> +
> + mbox_mcu_r5fss0_core1: mbox-mcu-r5fss0-core1 {
> + ti,mbox-tx = <1 0 0>;
> + ti,mbox-rx = <0 0 0>;
> + };
> };
>
> mailbox0_cluster2: mailbox@31f82000 {
> --
> 2.22.0
>
--
Regards,
Nishanth Menon
^ permalink raw reply
* Re: [PATCH 1/4] arm64: dts: ti: k3-am65-main: Add mailbox cluster nodes
From: Nishanth Menon @ 2019-07-23 11:35 UTC (permalink / raw)
To: Suman Anna; +Cc: Tero Kristo, devicetree, linux-arm-kernel
In-Reply-To: <20190722202024.14867-2-s-anna@ti.com>
On 15:20-20190722, Suman Anna wrote:
> The AM65x Main NavSS block contains a Mailbox IP instance with
> multiple clusters. Each cluster is equivalent to an Mailbox IP
> instance on OMAP platforms.
>
> Add all the Mailbox clusters as their own nodes under the MAIN
> NavSS cbass_main_navss interconnect node instead of creating an
> almost empty parent node for the new K3 mailbox IP and the clusters
> as its child nodes. All these nodes are marked as disabled, and
> they need to be enabled along with the appropriate child nodes
> on a need basis.
>
> NOTE:
> The NavSS only has a limited number of interrupts, so all the
> interrupts generated by a Mailbox IP are not added by default.
> Only the needed interrupts that are targeted towards the A53
> GIC will need to be be added later on when some sub-mailbox
> child nodes are added.
>
> Signed-off-by: Suman Anna <s-anna@ti.com>
> ---
> arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 108 +++++++++++++++++++++++
> 1 file changed, 108 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
> index 8413e80f9d3a..0b3ea2a871ee 100644
> --- a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
> @@ -419,6 +419,114 @@
> reg = <0x00 0x30e00000 0x00 0x1000>;
> #hwlock-cells = <1>;
> };
> +
> + mailbox0_cluster0: mailbox@31f80000 {
> + compatible = "ti,am654-mailbox";
> + reg = <0x00 0x31f80000 0x00 0x200>;
> + #mbox-cells = <1>;
> + ti,mbox-num-users = <4>;
> + ti,mbox-num-fifos = <16>;
> + status = "disabled";
We don't use status="disabled" as default so far.
--
Regards,
Nishanth Menon
^ permalink raw reply
* [PATCH 2/2] dt-bindings: IDU-intc: Add support for edge-triggered interrupts
From: Mischa Jonker @ 2019-07-23 10:26 UTC (permalink / raw)
To: Vineet.Gupta1, Alexey.Brodkin, kstewart, tglx, robh+dt,
linux-snps-arc, linux-kernel, devicetree
Cc: Mischa Jonker
In-Reply-To: <20190723102606.309089-1-mischa.jonker@synopsys.com>
This updates the documentation for supporting a optional extra interrupt
cell to specify edge vs level triggered.
Signed-off-by: Mischa Jonker <mischa.jonker@synopsys.com>
---
.../interrupt-controller/snps,archs-idu-intc.txt | 30 ++++++++++++++--------
1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/Documentation/devicetree/bindings/interrupt-controller/snps,archs-idu-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/snps,archs-idu-intc.txt
index 09fc02b..a5c1db9 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/snps,archs-idu-intc.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/snps,archs-idu-intc.txt
@@ -1,20 +1,30 @@
* ARC-HS Interrupt Distribution Unit
- This optional 2nd level interrupt controller can be used in SMP configurations for
- dynamic IRQ routing, load balancing of common/external IRQs towards core intc.
+ This optional 2nd level interrupt controller can be used in SMP configurations
+ for dynamic IRQ routing, load balancing of common/external IRQs towards core
+ intc.
Properties:
- compatible: "snps,archs-idu-intc"
- interrupt-controller: This is an interrupt controller.
-- #interrupt-cells: Must be <1>.
-
- Value of the cell specifies the "common" IRQ from peripheral to IDU. Number N
- of the particular interrupt line of IDU corresponds to the line N+24 of the
- core interrupt controller.
-
- intc accessed via the special ARC AUX register interface, hence "reg" property
- is not specified.
+- #interrupt-cells: Must be <1> or <2>.
+
+ Value of the first cell specifies the "common" IRQ from peripheral to IDU.
+ Number N of the particular interrupt line of IDU corresponds to the line N+24
+ of the core interrupt controller.
+
+ The (optional) second cell specifies any of the following flags:
+ - bits[3:0] trigger type and level flags
+ 1 = low-to-high edge triggered
+ 2 = NOT SUPPORTED (high-to-low edge triggered)
+ 4 = active high level-sensitive <<< DEFAULT
+ 8 = NOT SUPPORTED (active low level-sensitive)
+ When no second cell is specified, the interrupt is assumed to be level
+ sensitive.
+
+ The interrupt controller is accessed via the special ARC AUX register
+ interface, hence "reg" property is not specified.
Example:
core_intc: core-interrupt-controller {
--
2.8.3
^ permalink raw reply related
* [PATCH 1/2] ARCv2: IDU-intc: Add support for edge-triggered interrupts
From: Mischa Jonker @ 2019-07-23 10:26 UTC (permalink / raw)
To: Vineet.Gupta1, Alexey.Brodkin, kstewart, tglx, robh+dt,
linux-snps-arc, linux-kernel, devicetree
Cc: Mischa Jonker
This adds support for an optional extra interrupt cell to specify edge
vs level triggered. It is backward compatible with dts files with only
one cell, and will default to level-triggered in such a case.
Signed-off-by: Mischa Jonker <mischa.jonker@synopsys.com>
---
arch/arc/kernel/mcip.c | 56 ++++++++++++++++++++++++++++++++++++++++++++------
include/soc/arc/mcip.h | 11 ++++++++++
2 files changed, 61 insertions(+), 6 deletions(-)
diff --git a/arch/arc/kernel/mcip.c b/arch/arc/kernel/mcip.c
index 18b493d..fc45564 100644
--- a/arch/arc/kernel/mcip.c
+++ b/arch/arc/kernel/mcip.c
@@ -202,8 +202,8 @@ static void idu_set_dest(unsigned int cmn_irq, unsigned int cpu_mask)
__mcip_cmd_data(CMD_IDU_SET_DEST, cmn_irq, cpu_mask);
}
-static void idu_set_mode(unsigned int cmn_irq, unsigned int lvl,
- unsigned int distr)
+static void idu_set_mode(unsigned int cmn_irq, bool set_lvl, unsigned int lvl,
+ bool set_distr, unsigned int distr)
{
union {
unsigned int word;
@@ -212,8 +212,11 @@ static void idu_set_mode(unsigned int cmn_irq, unsigned int lvl,
};
} data;
- data.distr = distr;
- data.lvl = lvl;
+ data.word = __mcip_cmd_read(CMD_IDU_READ_MODE, cmn_irq);
+ if (set_distr)
+ data.distr = distr;
+ if (set_lvl)
+ data.lvl = lvl;
__mcip_cmd_data(CMD_IDU_SET_MODE, cmn_irq, data.word);
}
@@ -240,6 +243,25 @@ static void idu_irq_unmask(struct irq_data *data)
raw_spin_unlock_irqrestore(&mcip_lock, flags);
}
+static void idu_irq_ack(struct irq_data *data)
+{
+ unsigned long flags;
+
+ raw_spin_lock_irqsave(&mcip_lock, flags);
+ __mcip_cmd(CMD_IDU_ACK_CIRQ, data->hwirq);
+ raw_spin_unlock_irqrestore(&mcip_lock, flags);
+}
+
+static void idu_irq_mask_ack(struct irq_data *data)
+{
+ unsigned long flags;
+
+ raw_spin_lock_irqsave(&mcip_lock, flags);
+ __mcip_cmd_data(CMD_IDU_SET_MASK, data->hwirq, 1);
+ __mcip_cmd(CMD_IDU_ACK_CIRQ, data->hwirq);
+ raw_spin_unlock_irqrestore(&mcip_lock, flags);
+}
+
static int
idu_irq_set_affinity(struct irq_data *data, const struct cpumask *cpumask,
bool force)
@@ -263,13 +285,32 @@ idu_irq_set_affinity(struct irq_data *data, const struct cpumask *cpumask,
else
distribution_mode = IDU_M_DISTRI_RR;
- idu_set_mode(data->hwirq, IDU_M_TRIG_LEVEL, distribution_mode);
+ idu_set_mode(data->hwirq, false, 0, true, distribution_mode);
raw_spin_unlock_irqrestore(&mcip_lock, flags);
return IRQ_SET_MASK_OK;
}
+static int idu_irq_set_type(struct irq_data *data, u32 type)
+{
+ unsigned long flags;
+
+ if (type & ~(IRQ_TYPE_EDGE_RISING | IRQ_TYPE_LEVEL_HIGH))
+ return -EINVAL;
+
+ raw_spin_lock_irqsave(&mcip_lock, flags);
+
+ idu_set_mode(data->hwirq, true,
+ type & IRQ_TYPE_EDGE_RISING ? IDU_M_TRIG_EDGE :
+ IDU_M_TRIG_LEVEL,
+ false, 0);
+
+ raw_spin_unlock_irqrestore(&mcip_lock, flags);
+
+ return 0;
+}
+
static void idu_irq_enable(struct irq_data *data)
{
/*
@@ -289,7 +330,10 @@ static struct irq_chip idu_irq_chip = {
.name = "MCIP IDU Intc",
.irq_mask = idu_irq_mask,
.irq_unmask = idu_irq_unmask,
+ .irq_ack = idu_irq_ack,
+ .irq_mask_ack = idu_irq_mask_ack,
.irq_enable = idu_irq_enable,
+ .irq_set_type = idu_irq_set_type,
#ifdef CONFIG_SMP
.irq_set_affinity = idu_irq_set_affinity,
#endif
@@ -317,7 +361,7 @@ static int idu_irq_map(struct irq_domain *d, unsigned int virq, irq_hw_number_t
}
static const struct irq_domain_ops idu_irq_ops = {
- .xlate = irq_domain_xlate_onecell,
+ .xlate = irq_domain_xlate_onetwocell,
.map = idu_irq_map,
};
diff --git a/include/soc/arc/mcip.h b/include/soc/arc/mcip.h
index 50f49e0..d1a93c7 100644
--- a/include/soc/arc/mcip.h
+++ b/include/soc/arc/mcip.h
@@ -46,7 +46,9 @@ struct mcip_cmd {
#define CMD_IDU_ENABLE 0x71
#define CMD_IDU_DISABLE 0x72
#define CMD_IDU_SET_MODE 0x74
+#define CMD_IDU_READ_MODE 0x75
#define CMD_IDU_SET_DEST 0x76
+#define CMD_IDU_ACK_CIRQ 0x79
#define CMD_IDU_SET_MASK 0x7C
#define IDU_M_TRIG_LEVEL 0x0
@@ -119,4 +121,13 @@ static inline void __mcip_cmd_data(unsigned int cmd, unsigned int param,
__mcip_cmd(cmd, param);
}
+/*
+ * Read MCIP register
+ */
+static inline unsigned int __mcip_cmd_read(unsigned int cmd, unsigned int param)
+{
+ __mcip_cmd(cmd, param);
+ return read_aux_reg(ARC_REG_MCIP_READBACK);
+}
+
#endif
--
2.8.3
^ permalink raw reply related
* Re: [RFC,v3 6/9] media: platform: Add Mediatek ISP P1 V4L2 functions
From: Tomasz Figa @ 2019-07-23 10:21 UTC (permalink / raw)
To: Jungo Lin
Cc: devicetree, Sean Cheng (鄭昇弘),
Frederic Chen (陳俊元),
Rynn Wu (吳育恩), srv_heupstream, Rob Herring,
Ryan Yu (余孟修),
Frankie Chiu (邱文凱), Hans Verkuil, ddavenport,
Sj Huang, moderated list:ARM/Mediatek SoC support,
Laurent Pinchart, Matthias Brugger, Mauro Carvalho Chehab
In-Reply-To: <1563424741.1212.212.camel@mtksdccf07>
Hi Jungo,
On Thu, Jul 18, 2019 at 1:39 PM Jungo Lin <jungo.lin@mediatek.com> wrote:
>
> Hi, Tomasz:
>
> On Wed, 2019-07-10 at 18:54 +0900, Tomasz Figa wrote:
> > Hi Jungo,
> >
> > On Tue, Jun 11, 2019 at 11:53:41AM +0800, Jungo Lin wrote:
[snip]
> > > +static void mtk_cam_req_try_isp_queue(struct mtk_cam_dev *cam_dev,
> > > + struct media_request *new_req)
> > > +{
> > > + struct mtk_cam_dev_request *req, *req_safe, *cam_dev_req;
> > > + struct device *dev = &cam_dev->pdev->dev;
> > > +
> > > + dev_dbg(dev, "%s new req:%d", __func__, !new_req);
> > > +
> > > + if (!cam_dev->streaming) {
> > > + cam_dev_req = mtk_cam_req_to_dev_req(new_req);
> > > + spin_lock(&cam_dev->req_lock);
> > > + list_add_tail(&cam_dev_req->list, &cam_dev->req_list);
> > > + spin_unlock(&cam_dev->req_lock);
> > > + dev_dbg(dev, "%s: stream off, no ISP enqueue\n", __func__);
> > > + return;
> > > + }
> > > +
> > > + /* Normal enqueue flow */
> > > + if (new_req) {
> > > + mtk_isp_req_enqueue(dev, new_req);
> > > + return;
> > > + }
> > > +
> > > + /* Flush all media requests wehen first stream on */
> > > + list_for_each_entry_safe(req, req_safe, &cam_dev->req_list, list) {
> > > + list_del(&req->list);
> > > + mtk_isp_req_enqueue(dev, &req->req);
> > > + }
> > > +}
> >
> > This will have to be redone, as per the other suggestions, but generally one
> > would have a function that tries to queue as much as possible from a list to
> > the hardware and another function that adds a request to the list and calls
> > the first function.
> >
>
> We revised this function as below.
> First to check the en-queue conditions:
> a. stream on
> b. The composer buffers in SCP are 3, so we only could has 3 jobs
> at the same time.
>
>
> Second, try to en-queue the frames in the pending job if possible and
> move them into running job list if possible.
>
> The request has been inserted into pending job in mtk_cam_req_validate
> which is used to validate media_request.
Thanks for replying to each of the comments, that's very helpful.
Snipped out the parts that I agreed with.
Please note that req_validate is not supposed to change any driver
state. It's only supposed to validate the request. req_queue is the
right callback to insert the request into some internal driver
bookkeeping structures.
>
> void mtk_cam_dev_req_try_queue(struct mtk_cam_dev *cam_dev)
> {
> struct mtk_cam_dev_request *req, *req_prev;
> struct list_head enqueue_job_list;
> int buffer_cnt = atomic_read(&cam_dev->running_job_count);
> unsigned long flags;
>
> if (!cam_dev->streaming ||
> buffer_cnt >= MTK_ISP_MAX_RUNNING_JOBS) {
Do we have a guarantee that cam_dev->running_job_count doesn't
decrement between the atomic_read() above and this line?
> dev_dbg(cam_dev->dev, "stream off or buffers are full:%d\n",
> buffer_cnt);
> return;
> }
>
> INIT_LIST_HEAD(&enqueue_job_list);
>
> spin_lock(&cam_dev->pending_job_lock);
> list_for_each_entry_safe(req, req_prev,
> &cam_dev->pending_job_list, list) {
> list_del(&req->list);
> list_add_tail(&req->list, &enqueue_job_list);
What's the reason to use the second list? Could we just take one job
from pending_job_list, enqueue it and then iterate again?
> if (atomic_inc_return(&cam_dev->running_job_count) >=
> MTK_ISP_MAX_RUNNING_JOBS)
> break;
> }
> spin_unlock(&cam_dev->pending_job_lock);
>
> list_for_each_entry_safe(req, req_prev,
> &enqueue_job_list, list) {
> list_del(&req->list);
> spin_lock_irqsave(&cam_dev->running_job_lock, flags);
> list_add_tail(&req->list, &cam_dev->running_job_list);
> spin_unlock_irqrestore(&cam_dev->running_job_lock, flags);
>
Do we have a guarantee that another thread doesn't run the same
function ending up calling mtk_isp_req_enqueue() with another request
before this one and thus making the order of running_job_list
incorrect?
> mtk_isp_req_enqueue(cam_dev, req);
> }
> }
>
[snip]
> > > + stride = DIV_ROUND_UP(stride * pixel_byte, 8);
> > > +
> > > + if (pix_fmt == V4L2_PIX_FMT_MTISP_F10)
> > > + stride = ALIGN(stride, 4);
> >
> > Is it expected that only the F10 format needs this alignment?
> >
>
> yes, if the pixel bits of image format is 10, the byte alignment of bpl
> should be 4. Otherwise, it is 8. We will revise this and add more
> comments.
That means that the B10 format also needs the extra alignment, as
opposed to what the original code did, right?
>
> /* 4 bytes alignment for 10 bit other are 8 bytes alignment */
> if (pixel_bits == 10)
> bpl = ALIGN(bpl, 4);
> else
> bpl = ALIGN(bpl, 8);
SGTM, thanks.
[snip]
> > > +
> > > +static struct v4l2_subdev *
> > > +mtk_cam_cio_get_active_sensor(struct mtk_cam_dev *cam_dev)
> > > +{
> > > + struct media_device *mdev = cam_dev->seninf->entity.graph_obj.mdev;
> > > + struct media_entity *entity;
> > > + struct device *dev = &cam_dev->pdev->dev;
> > > + struct v4l2_subdev *sensor;
> >
> > This variable would be unitialized if there is no streaming sensor. Was
> > there no compiler warning generated for this?
> >
>
> No, there is no compiler warning.
> But, we will assign sensor to NULL to avoid unnecessary compiler warning
> with different compiler options.
>
Thanks. It would be useful if you could check why the compiler you're
using doesn't show a warning here. We might be missing other
uninitialized variables.
[snip]
> > > +}
> > > +
> > > +static int mtk_cam_media_link_setup(struct media_entity *entity,
> > > + const struct media_pad *local,
> > > + const struct media_pad *remote, u32 flags)
> > > +{
> > > + struct mtk_cam_dev *cam_dev =
> > > + container_of(entity, struct mtk_cam_dev, subdev.entity);
> > > + u32 pad = local->index;
> > > +
> > > + dev_dbg(&cam_dev->pdev->dev, "%s: %d -> %d flags:0x%x\n",
> > > + __func__, pad, remote->index, flags);
> > > +
> > > + if (pad < MTK_CAM_P1_TOTAL_NODES)
> >
> > I assume this check is needed, because the pads with higher indexes are not
> > video nodes? If so, a comment would be helpful here.
> >
>
> Yes, we will new comment as below.
>
> /*
> * Check video nodes is enabled by link setup.
> * The pad index of video node should be less than
> * MTK_CAM_P1_TOTAL_NODES.
> */
> if (pad < MTK_CAM_P1_TOTAL_NODES)
> cam_dev->vdev_nodes[pad].enabled =
> !!(flags & MEDIA_LNK_FL_ENABLED);
>
Could we rephrase this a bit. The comment still doesn't explain why
the index should be less than the constant. Perhaps:
/*
* The video nodes exposed by the driver have pads indexes
* from 0 to MTK_CAM_P1_TOTAL_NODES - 1.
*/
[snip]
> > > +
> > > + dev_dbg(dev, "%s: node:%d fd:%d idx:%d\n",
> > > + __func__,
> > > + node->id,
> > > + buf->vbb.request_fd,
> > > + buf->vbb.vb2_buf.index);
> > > +
> > > + /* For request buffers en-queue, handled in mtk_cam_req_try_queue */
> > > + if (vb->vb2_queue->uses_requests)
> > > + return;
> >
> > I'd suggest removing non-request support from this driver. Even if we end up
> > with a need to provide compatibility for non-request mode, then it should be
> > built on top of the requests mode, so that the driver itself doesn't have to
> > deal with two modes.
> >
>
> The purpose of non-request function in this driver is needed by
> our camera middle-ware design. It needs 3A statistics buffers before
> image buffers en-queue. So we need to en-queue 3A statistics with
> non-request mode in this driver. After MW got the 3A statistics data, it
> will en-queue the images, tuning buffer and other meta buffers with
> request mode. Based on this requirement, do you have any suggestion?
> For upstream driver, should we only consider request mode?
>
Where does that requirement come from? Why the timing of queuing of
the buffers to the driver is important?
[snip]
> > > +static int mtk_cam_vb2_start_streaming(struct vb2_queue *vq,
> > > + unsigned int count)
> > > +{
> > > + struct mtk_cam_dev *cam_dev = vb2_get_drv_priv(vq);
> > > + struct mtk_cam_video_device *node = mtk_cam_vbq_to_vdev(vq);
> > > + struct device *dev = &cam_dev->pdev->dev;
> > > + unsigned int node_count = cam_dev->subdev.entity.use_count;
> > > + int ret;
> > > +
> > > + if (!node->enabled) {
> >
> > How is this synchronized with mtk_cam_media_link_setup()?
> >
>
> We will follow your suggestion and below is our proposal for this
> function.
>
> 1. Use !cam_dev->pipeline.streaming_count to decide the first node to
> stream-on.
> 2.a If yes, do the following steps
> 2.a-1 Call media_pipeline_start function to prevent the link
> configuration changes.
> 2.a-2 Call mtk_cam_dev_init_stream function to calculate how many
> video nodes are enabled and save it into cam_dev->enabled_node_count.
> 2.a-3 Initialize ISP P1 HW in mtk_isp_hw_init function since end
> user has called stream-on API
> 2.b jump step 3.
>
> 3. Use cam_dev->streamed_node_count to track how many video nodes are
> streamed by user space.
> 4. Check all enabled video nodes are streamed or not based on
> cam_dev->streamed_node_count & cam_dev->enabled_node_count.
> 5. If yes, call s_stream on for P1 sub-device
>
> Do you think it is reasonable?
>
That should work indeed.
[snip]
> > > +
> > > + mtk_cam_vb2_return_all_buffers(cam_dev, node, VB2_BUF_STATE_ERROR);
> >
> > Shouldn't we stop streaming first, so that the hardware operation is
> > cancelled and any buffers owned by the hardware are released?
> >
>
> For this function, below is the new code flow.
>
> 1. Check the first node to stream off based on
> cam_dev->streamed_node_count & cam_dev->enabled_node_count.
> 2. If yes, call all s_stream off for P1 sub-device
> 3. Call mtk_cam_vb2_return_all_buffers for each node
> 4. Check the last node to stream off
> 5. If yes, call media_pipeline_stop to allow user space
> to perform link configuration changes, such as disable link.
>
> But, for step 5, is it too late for end user to disable link?
> For example, for first node, it has called stream off but
> can't call disable link until the last node is stream off?
>
I think that should be okay. From the userspace point of view, having
one of the video nodes streaming implies that the related subdevice
could be streaming as well. The links between the video nodes and the
subdevices don't have the DYNAMIC flag, so the userspace should expect
that it can't change any links connecting to the subdevice when the
subdevice could be streaming.
[snip]
> > > +static void mtk_cam_vb2_buf_request_complete(struct vb2_buffer *vb)
> > > +{
> > > + struct mtk_cam_dev *dev = vb2_get_drv_priv(vb->vb2_queue);
> > > +
> > > + v4l2_ctrl_request_complete(vb->req_obj.req,
> > > + dev->v4l2_dev.ctrl_handler);
> >
> > This would end up being called multiple times, once for each video node.
> > Instead, this should be called explicitly by the driver when it completed
> > the request - perhaps in the frame completion handler?
> >
> > With that, we probably wouldn't even need this callback.
> >
>
> First, if we don't implement this callback function, we will receive
> kernel warning as below.
>
> https://elixir.bootlin.com/linux/latest/source/drivers/media/common/videobuf2/videobuf2-v4l2.c#L420
>
> Second, this function is only be called in __vb2_queue_cancel function.
> Moreover, we will remove cam_dev->v4l2_dev.ctrl_handler in next patch.
> So could we just implement dummy empty function?
>
> * @buf_request_complete: a buffer that was never queued to the driver
> but is
> * associated with a queued request was canceled.
> * The driver will have to mark associated objects in the
> * request as completed; required if requests are
> * supported.
>
Good catch, thanks.
Sounds like we may indeed need to implement this callback. In
particular, we may need to remove the request that the buffer was
associated with from the driver queue and return the other buffers
associated to it with an error state. This should be similar to
handling a request failure.
[snip]
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int mtk_cam_vidioc_enum_fmt(struct file *file, void *fh,
> > > + struct v4l2_fmtdesc *f)
> > > +{
> > > + struct mtk_cam_video_device *node = file_to_mtk_cam_node(file);
> > > +
> > > + if (f->index >= node->desc.num_fmts)
> > > + return -EINVAL;
> > > +
> > > + f->pixelformat = node->desc.fmts[f->index].fmt.pix_mp.pixelformat;
> >
> > Is the set of formats available always the same regardless of the sensor
> > format?
> >
>
> Yes, ISP P1 HW output formats are always available without impact
> by sensor formats.
>
> > > + f->flags = 0;
> >
> > We need f->description too.
> >
>
> For this description, do you suggest 1). we fill this field in this
> function or 2). v4l_fill_fmtdesc function in v4l2-ioctl?
>
> https://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core/v4l2-ioctl.c#L1152
>
> Basically, we prefer method 1.
>
That should be v4l_fill_fmtdesc(), as it already includes other
vendor-specific formats.
[snip]
> > > +
> > > + dev_dbg(&cam_dev->pdev->dev, "%s: fmt:%c%c%c%c, w*h:%u*%u\n",
> > > + __func__,
> > > + (in_fmt->fmt.pix_mp.pixelformat & 0xFF),
> > > + (in_fmt->fmt.pix_mp.pixelformat >> 8) & 0xFF,
> > > + (in_fmt->fmt.pix_mp.pixelformat >> 16) & 0xFF,
> > > + (in_fmt->fmt.pix_mp.pixelformat >> 24) & 0xFF,
> > > + in_fmt->fmt.pix_mp.width, in_fmt->fmt.pix_mp.height);
> > > +
> > > + width = in_fmt->fmt.pix_mp.width;
> > > + height = in_fmt->fmt.pix_mp.height;
> > > +
> > > + dev_fmt = mtk_cam_dev_find_fmt(&node->desc,
> > > + in_fmt->fmt.pix_mp.pixelformat);
> > > + if (dev_fmt) {
> > > + mtk_cam_dev_set_img_fmt(&cam_dev->pdev->dev,
> > > + &in_fmt->fmt.pix_mp,
> > > + &dev_fmt->fmt.pix_mp,
> > > + node->id);
> > > + } else {
> > > + mtk_cam_dev_load_default_fmt(&cam_dev->pdev->dev,
> > > + &node->desc, in_fmt);
> >
> > We shouldn't just load a default format. This function should validate all
> > the fields one by one and adjust them to something appropriate.
> >
>
> For ISP P1 HW, we only cares these fields of v4l2_pix_format_mplane.
> a. width
> b. height
> c. pixelformat
> d. plane_fmt
> - sizeimage
> - bytesperline
> e. num_planes
> Other fields are consider constant.
>
> So if the user space passes one pixel format with un-supported, we will
> apply the default format firstly and adjust width, height, sizeimage,
> and bytesperline. We will focus on validate width & height.
> Is it ok?
I'm not sure I understand your proposal, but let me describe the
proper behavior here:
if (pixelformat is invalid)
pixelformat = some valid pixel format;
width = clamp(width, driver min, driver max);
height = clamp(height, driver min, driver max);
num_planes = 1;
calculate_sizeimage_and_bytesperline(fmt);
fill_in_the_remaining_constant_fields(fmt);
Does it make sense?
[snip]
> > > +static int mtk_cam_vidioc_s_fmt(struct file *file, void *fh,
> > > + struct v4l2_format *f)
> > > +{
> > > + struct mtk_cam_dev *cam_dev = video_drvdata(file);
> > > + struct mtk_cam_video_device *node = file_to_mtk_cam_node(file);
> > > +
> > > + if (cam_dev->streaming)
> > > + return -EBUSY;
> >
> > I think this should rather be something like vb2_queue_is_busy(), which
> > would prevent format changes if buffers are allocated.
> >
>
> Since vb2_queue_is_busy is static function, would we paste its
> implementation in this function to check like this?
>
> if (node->vdev.queue->owner &&
> node->vdev.queue->owner != file->private_data) {
> dev_err(cam_dev->dev, "%s err: buffer allocated\n", __func__);
> return -EBUSY;
> }
>
Sorry, I mixed up the function name. That should've been vb2_is_busy().
[snip]
> > > + /* Total pad numbers is video devices + one seninf pad */
> > > + unsigned int num_subdev_pads = MTK_CAM_CIO_PAD_SINK + 1;
> > > + unsigned int i;
> > > + int ret;
> > > +
> > > + ret = mtk_cam_media_register(dev,
> > > + &cam_dev->media_dev);
> > > + if (ret) {
> > > + dev_err(dev, "failed to register media device:%d\n", ret);
> > > + return ret;
> > > + }
> > > + dev_info(dev, "Register media device: %s, 0x%pK",
> > > + MTK_CAM_DEV_P1_NAME, cam_dev->media_dev);
> >
> > An info message should be useful to the user in some way. Printing kernel
> > pointers isn't useful. Something like "registered media0" could be useful to
> > let the user know which media device is associated with this driver if there
> > is more than one in the system.
> >
>
> Here is the new log info.
>
> dev_info(dev, "media%d register",cam->media_dev.devnode->minor);
>
Let's fix the missing space and making a bit more readable:
dev_info(dev, "Registered media%d", cam->media_dev.devnode->minor);
>
> > > +
> > > + /* Set up v4l2 device */
> > > + cam_dev->v4l2_dev.mdev = &cam_dev->media_dev;
> > > + ret = v4l2_device_register(dev, &cam_dev->v4l2_dev);
> > > + if (ret) {
> > > + dev_err(dev, "failed to register V4L2 device:%d\n", ret);
> > > + goto fail_v4l2_dev;
> >
> > Please call the labels after the cleanup step that needs to be done. It
> > makes it easier to spot any ordering errors.
> >
>
> Will fix in next patch.
>
> > > + }
> > > + dev_info(dev, "Register v4l2 device: 0x%pK", cam_dev->v4l2_dev);
> >
> > Same as above.
> >
>
> Ditto.
>
> dev_info(dev, "Register v4l2 device: %s", cam->v4l2_dev.name);
>
Perhaps just "Registered %s" to be consistent with the above media log?
[snip]
> > > +
> > > +static int mtk_cam_dev_notifier_bound(struct v4l2_async_notifier *notifier,
> > > + struct v4l2_subdev *sd,
> > > + struct v4l2_async_subdev *asd)
> > > +{
> > > + struct mtk_cam_dev *cam_dev =
> > > + container_of(notifier, struct mtk_cam_dev, notifier);
> > > +
> >
> > Should we somehow check that the entity we got is seninf indeed and there
> > was no mistake in DT?
> >
>
> How about to check the entity function of seninf device?
>
> if (!(sd->entity.function & MEDIA_ENT_F_VID_IF_BRIDGE)) {
> dev_dbg(cam->dev, "No MEDIA_ENT_F_VID_IF_BRIDGE function\n");
> return -ENODEV;
> }
>
> If we need to check DT, may we need to implement this in parse_endpoint
> callback function of v4l2_async_notifier_parse_fwnode_endpoints?
>
Yes, checking the entity function is indeed the right way to do this.
[snip]
> > > + .default_fmt_idx = 4,
> > > + .max_buf_count = 10,
> >
> > Where does this number come from?
> >
>
> The default maximum VB2 buffer count is 32.
> In order to limit memory usage, we like to limit the maximum buffer
> counts in the driver layer. The maximum buffer count is estimated
> according to our camera MW.
>
> #define VB2_MAX_FRAME (32)
>
Okay, thanks.
[snip]
> > > + struct media_pad vdev_pad;
> > > + struct vb2_queue vbq;
> > > + struct v4l2_ctrl_handler ctrl_handler;
> > > + struct list_head pending_list;
> > > + /* Used for vbq & vdev */
> >
> > It's already documented in the kerneldoc comment.
> >
>
> Fixed in next patch.
> Btw, if we remove this, we will got complain from checkpatch.pl script.
>
Oh really, that's weird. Okay, please keep it then, sorry for the confusion.
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH v2 02/10] iio: document bindings for mounting matrices
From: H. Nikolaus Schaller @ 2019-07-23 9:46 UTC (permalink / raw)
To: Linus Walleij
Cc: Jonathan Cameron, Rob Herring, Mark Rutland, Andy Shevchenko,
Charles Keepax, Song Qiang, Jean-Baptiste Maneyrol, Martin Kelly,
Jonathan Marek, Brian Masney, Stephan Gerhold,
Discussions about the Letux Kernel, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
In-Reply-To: <CACRpkdZ5Z9VY457Fywt6X=K5XONgiPVcwbwSkwL_U+GCqZ+u5g@mail.gmail.com>
Hi Linus,
> Am 23.07.2019 um 09:42 schrieb Linus Walleij <linus.walleij@linaro.org>:
>
> Hi H. Nikolaus,
>
> On Thu, Feb 21, 2019 at 6:03 PM H. Nikolaus Schaller <hns@goldelico.com> wrote:
>
>> From: Linus Walleij <linus.walleij@linaro.org>
>
> It is fair for you to change authorship to yourself at this point.
> Just keeping my Signed-off-by is sufficient.
Well, I think my contribution is less than yours :)
>
>> The mounting matrix for sensors was introduced in
>> commit dfc57732ad38 ("iio:core: mounting matrix support")
>>
>> However the device tree bindings are very terse and since this is
>> a widely applicable property, we need a proper binding for it
>> that the other bindings can reference. This will also be useful
>> for other operating systems and sensor engineering at large.
>>
>> I think all 3D sensors should support it, the current situation
>> is probably that the mounting information is confined in magic
>> userspace components rather than using the mounting matrix, which
>> is not good for portability and reuse.
>>
>> Cc: Linus Walleij <linus.walleij@linaro.org>
>> Cc: Gregor Boirie <gregor.boirie@parrot.com>
>> Cc: Sebastian Reichel <sre@kernel.org>
>> Cc: Samu Onkalo <samu.onkalo@intel.com>
>> Cc: devicetree@vger.kernel.org
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>
> Did this patch fall off somewhere? I think it's really neat, even in this
> form it is great help for developers. If you want I can try picking up the
> comments and resend it.
Well, I had planned to review it again and promised to send out a new
version. But as usual this ToDo becomes hidden by always more important
tasks.
So I am fine if you can pick comments and resend it. I think there will
be others who help to make it even better in the future if the mount matrix
is more widely used.
BR,
Nikolaus
^ permalink raw reply
* RE: [PATCH v3 6/8] PCI: al: Add support for DW based driver type
From: Gustavo Pimentel @ 2019-07-23 9:40 UTC (permalink / raw)
To: Jonathan Chocron, lorenzo.pieralisi@arm.com, bhelgaas@google.com,
jingoohan1@gmail.com, gustavo.pimentel@synopsys.com,
robh+dt@kernel.org, mark.rutland@arm.com
Cc: dwmw@amazon.co.uk, benh@kernel.crashing.org, alisaidi@amazon.com,
ronenk@amazon.com, barakw@amazon.com, talel@amazon.com,
hanochu@amazon.com, hhhawa@amazon.com, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
In-Reply-To: <20190723092711.11786-2-jonnyc@amazon.com>
On Tue, Jul 23, 2019 at 10:27:9, Jonathan Chocron <jonnyc@amazon.com>
wrote:
> This driver is DT based and utilizes the DesignWare APIs.
> It allows using a smaller ECAM range for a larger bus range -
> usually an entire bus uses 1MB of address space, but the driver
> can use it for a larger number of buses.
>
> All link initializations are handled by the boot FW.
>
> Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>
> ---
> drivers/pci/controller/dwc/Kconfig | 12 +
> drivers/pci/controller/dwc/pcie-al.c | 367 +++++++++++++++++++++++++++
> 2 files changed, 379 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index 6ea778ae4877..3c6094cbcc3b 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -230,4 +230,16 @@ config PCIE_UNIPHIER
> Say Y here if you want PCIe controller support on UniPhier SoCs.
> This driver supports LD20 and PXs3 SoCs.
>
> +config PCIE_AL
> + bool "Amazon Annapurna Labs PCIe controller"
> + depends on OF && (ARM64 || COMPILE_TEST)
> + depends on PCI_MSI_IRQ_DOMAIN
> + select PCIE_DW_HOST
> + help
> + Say Y here to enable support of the Amazon's Annapurna Labs PCIe
> + controller IP on Amazon SoCs. The PCIe controller uses the DesignWare
> + core plus Annapurna Labs proprietary hardware wrappers. This is
> + required only for DT-based platforms. ACPI platforms with the
> + Annapurna Labs PCIe controller don't need to enable this.
> +
> endmenu
> diff --git a/drivers/pci/controller/dwc/pcie-al.c b/drivers/pci/controller/dwc/pcie-al.c
> index 3ab58f0584a8..3ffdd3c97617 100644
> --- a/drivers/pci/controller/dwc/pcie-al.c
> +++ b/drivers/pci/controller/dwc/pcie-al.c
> @@ -91,3 +91,370 @@ struct pci_ecam_ops al_pcie_ops = {
> };
>
> #endif /* defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS) */
> +
> +#ifdef CONFIG_PCIE_AL
> +
> +#include <linux/of_pci.h>
> +#include "pcie-designware.h"
> +
> +#define AL_PCIE_REV_ID_2 2
> +#define AL_PCIE_REV_ID_3 3
> +#define AL_PCIE_REV_ID_4 4
> +
> +#define AXI_BASE_OFFSET 0x0
> +
> +#define DEVICE_ID_OFFSET 0x16c
> +
> +#define DEVICE_REV_ID 0x0
> +#define DEVICE_REV_ID_DEV_ID_MASK GENMASK(31, 16)
> +
> +#define DEVICE_REV_ID_DEV_ID_X4 0
> +#define DEVICE_REV_ID_DEV_ID_X8 2
> +#define DEVICE_REV_ID_DEV_ID_X16 4
> +
> +#define OB_CTRL_REV1_2_OFFSET 0x0040
> +#define OB_CTRL_REV3_5_OFFSET 0x0030
> +
> +#define CFG_TARGET_BUS 0x0
> +#define CFG_TARGET_BUS_MASK_MASK GENMASK(7, 0)
> +#define CFG_TARGET_BUS_BUSNUM_MASK GENMASK(15, 8)
> +
> +#define CFG_CONTROL 0x4
> +#define CFG_CONTROL_SUBBUS_MASK GENMASK(15, 8)
> +#define CFG_CONTROL_SEC_BUS_MASK GENMASK(23, 16)
> +
> +struct al_pcie_reg_offsets {
> + unsigned int ob_ctrl;
> +};
> +
> +struct al_pcie_target_bus_cfg {
> + u8 reg_val;
> + u8 reg_mask;
> + u8 ecam_mask;
> +};
> +
> +struct al_pcie {
> + struct dw_pcie *pci;
> + void __iomem *controller_base; /* base of PCIe unit (not DW core) */
> + struct device *dev;
> + resource_size_t ecam_size;
> + unsigned int controller_rev_id;
> + struct al_pcie_reg_offsets reg_offsets;
> + struct al_pcie_target_bus_cfg target_bus_cfg;
> +};
> +
> +#define PCIE_ECAM_DEVFN(x) (((x) & 0xff) << 12)
> +
> +#define to_al_pcie(x) dev_get_drvdata((x)->dev)
> +
> +static inline u32 al_pcie_controller_readl(struct al_pcie *pcie, u32 offset)
> +{
> + return readl(pcie->controller_base + offset);
> +}
> +
> +static inline void al_pcie_controller_writel(struct al_pcie *pcie, u32 offset,
> + u32 val)
> +{
> + writel(val, pcie->controller_base + offset);
> +}
> +
> +static int al_pcie_rev_id_get(struct al_pcie *pcie, unsigned int *rev_id)
> +{
> + u32 dev_rev_id_val;
> + u32 dev_id_val;
> +
> + dev_rev_id_val = al_pcie_controller_readl(pcie, AXI_BASE_OFFSET +
> + DEVICE_ID_OFFSET +
> + DEVICE_REV_ID);
> + dev_id_val = FIELD_GET(DEVICE_REV_ID_DEV_ID_MASK, dev_rev_id_val);
> +
> + switch (dev_id_val) {
> + case DEVICE_REV_ID_DEV_ID_X4:
> + *rev_id = AL_PCIE_REV_ID_2;
> + break;
> + case DEVICE_REV_ID_DEV_ID_X8:
> + *rev_id = AL_PCIE_REV_ID_3;
> + break;
> + case DEVICE_REV_ID_DEV_ID_X16:
> + *rev_id = AL_PCIE_REV_ID_4;
> + break;
> + default:
> + dev_err(pcie->dev, "Unsupported dev_id_val (0x%x)\n",
> + dev_id_val);
> + return -EINVAL;
> + }
> +
> + dev_dbg(pcie->dev, "dev_id_val: 0x%x\n", dev_id_val);
> +
> + return 0;
> +}
> +
> +static int al_pcie_reg_offsets_set(struct al_pcie *pcie)
> +{
> + switch (pcie->controller_rev_id) {
> + case AL_PCIE_REV_ID_2:
> + pcie->reg_offsets.ob_ctrl = OB_CTRL_REV1_2_OFFSET;
> + break;
> + case AL_PCIE_REV_ID_3:
> + case AL_PCIE_REV_ID_4:
> + pcie->reg_offsets.ob_ctrl = OB_CTRL_REV3_5_OFFSET;
> + break;
> + default:
> + dev_err(pcie->dev, "Unsupported controller rev_id: 0x%x\n",
> + pcie->controller_rev_id);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static inline void al_pcie_target_bus_set(struct al_pcie *pcie,
> + u8 target_bus,
> + u8 mask_target_bus)
> +{
> + u32 reg;
> +
> + reg = FIELD_PREP(CFG_TARGET_BUS_MASK_MASK, mask_target_bus) |
> + FIELD_PREP(CFG_TARGET_BUS_BUSNUM_MASK, target_bus);
> +
> + al_pcie_controller_writel(pcie, AXI_BASE_OFFSET +
> + pcie->reg_offsets.ob_ctrl + CFG_TARGET_BUS,
> + reg);
> +}
> +
> +static void __iomem *al_pcie_conf_addr_map(struct al_pcie *pcie,
> + unsigned int busnr,
> + unsigned int devfn)
> +{
> + struct al_pcie_target_bus_cfg *target_bus_cfg = &pcie->target_bus_cfg;
> + unsigned int busnr_ecam = busnr & target_bus_cfg->ecam_mask;
> + unsigned int busnr_reg = busnr & target_bus_cfg->reg_mask;
> + struct pcie_port *pp = &pcie->pci->pp;
> + void __iomem *pci_base_addr;
> +
> + pci_base_addr = (void __iomem *)((uintptr_t)pp->va_cfg0_base +
> + (busnr_ecam << 20) +
> + PCIE_ECAM_DEVFN(devfn));
> +
> + if (busnr_reg != target_bus_cfg->reg_val) {
> + dev_dbg(pcie->pci->dev, "Changing target bus busnum val from 0x%x to 0x%x\n",
> + target_bus_cfg->reg_val, busnr_reg);
> + target_bus_cfg->reg_val = busnr_reg;
> + al_pcie_target_bus_set(pcie,
> + target_bus_cfg->reg_val,
> + target_bus_cfg->reg_mask);
> + }
> +
> + return pci_base_addr;
> +}
> +
> +static int al_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
> + unsigned int devfn, int where, int size,
> + u32 *val)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct al_pcie *pcie = to_al_pcie(pci);
> + unsigned int busnr = bus->number;
> + void __iomem *pci_addr;
> + int rc;
> +
> + pci_addr = al_pcie_conf_addr_map(pcie, busnr, devfn);
> +
> + rc = dw_pcie_read(pci_addr + where, size, val);
> +
> + dev_dbg(pci->dev, "%d-byte config read from %04x:%02x:%02x.%d offset 0x%x (pci_addr: 0x%px) - val:0x%x\n",
> + size, pci_domain_nr(bus), bus->number,
> + PCI_SLOT(devfn), PCI_FUNC(devfn), where,
> + (pci_addr + where), *val);
> +
> + return rc;
> +}
> +
> +static int al_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus,
> + unsigned int devfn, int where, int size,
> + u32 val)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct al_pcie *pcie = to_al_pcie(pci);
> + unsigned int busnr = bus->number;
> + void __iomem *pci_addr;
> + int rc;
> +
> + pci_addr = al_pcie_conf_addr_map(pcie, busnr, devfn);
> +
> + rc = dw_pcie_write(pci_addr + where, size, val);
> +
> + dev_err(pci->dev, "%d-byte config write to %04x:%02x:%02x.%d offset 0x%x (pci_addr: 0x%px) - val:0x%x\n",
> + size, pci_domain_nr(bus), bus->number,
> + PCI_SLOT(devfn), PCI_FUNC(devfn), where,
> + (pci_addr + where), val);
> +
> + return rc;
> +}
> +
> +static void al_pcie_config_prepare(struct al_pcie *pcie)
> +{
> + struct al_pcie_target_bus_cfg *target_bus_cfg;
> + struct pcie_port *pp = &pcie->pci->pp;
> + unsigned int ecam_bus_mask;
> + u32 cfg_control_offset;
> + u8 subordinate_bus;
> + u8 secondary_bus;
> + u32 cfg_control;
> + u32 reg;
> +
> + target_bus_cfg = &pcie->target_bus_cfg;
> +
> + ecam_bus_mask = (pcie->ecam_size >> 20) - 1;
> + if (ecam_bus_mask > 255) {
> + dev_warn(pcie->dev, "ECAM window size is larger than 256MB. Cutting off at 256\n");
> + ecam_bus_mask = 255;
> + }
> +
> + /* This portion is taken from the transaction address */
> + target_bus_cfg->ecam_mask = ecam_bus_mask;
> + /* This portion is taken from the cfg_target_bus reg */
> + target_bus_cfg->reg_mask = ~target_bus_cfg->ecam_mask;
> + target_bus_cfg->reg_val = pp->busn->start & target_bus_cfg->reg_mask;
> +
> + al_pcie_target_bus_set(pcie, target_bus_cfg->reg_val,
> + target_bus_cfg->reg_mask);
> +
> + secondary_bus = pp->busn->start + 1;
> + subordinate_bus = pp->busn->end;
> +
> + /* Set the valid values of secondary and subordinate buses */
> + cfg_control_offset = AXI_BASE_OFFSET + pcie->reg_offsets.ob_ctrl +
> + CFG_CONTROL;
> +
> + cfg_control = al_pcie_controller_readl(pcie, cfg_control_offset);
> +
> + reg = cfg_control &
> + ~(CFG_CONTROL_SEC_BUS_MASK | CFG_CONTROL_SUBBUS_MASK);
> +
> + reg |= FIELD_PREP(CFG_CONTROL_SUBBUS_MASK, subordinate_bus) |
> + FIELD_PREP(CFG_CONTROL_SEC_BUS_MASK, secondary_bus);
> +
> + al_pcie_controller_writel(pcie, cfg_control_offset, reg);
> +}
> +
> +static int al_pcie_host_init(struct pcie_port *pp)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct al_pcie *pcie = to_al_pcie(pci);
> + int link_up;
> + int rc;
> +
> + rc = al_pcie_rev_id_get(pcie, &pcie->controller_rev_id);
> + if (rc)
> + return rc;
> +
> + rc = al_pcie_reg_offsets_set(pcie);
> + if (rc)
> + return rc;
> +
> + al_pcie_config_prepare(pcie);
> +
> + return 0;
> +}
> +
> +static const struct dw_pcie_host_ops al_pcie_host_ops = {
> + .rd_other_conf = al_pcie_rd_other_conf,
> + .wr_other_conf = al_pcie_wr_other_conf,
> + .host_init = al_pcie_host_init,
> +};
> +
> +static int al_add_pcie_port(struct pcie_port *pp,
> + struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + int ret;
> +
> + pp->ops = &al_pcie_host_ops;
> +
> + ret = dw_pcie_host_init(pp);
> + if (ret) {
> + dev_err(dev, "failed to initialize host\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static const struct dw_pcie_ops dw_pcie_ops = {
> +};
> +
> +static int al_pcie_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct resource *controller_res;
> + struct resource *ecam_res;
> + struct resource *dbi_res;
> + struct al_pcie *al_pcie;
> + struct dw_pcie *pci;
> + int ret;
> +
> + al_pcie = devm_kzalloc(dev, sizeof(*al_pcie), GFP_KERNEL);
> + if (!al_pcie)
> + return -ENOMEM;
> +
> + pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
> + if (!pci)
> + return -ENOMEM;
> +
> + pci->dev = dev;
> + pci->ops = &dw_pcie_ops;
> +
> + al_pcie->pci = pci;
> + al_pcie->dev = dev;
> +
> + dbi_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
> + pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_res);
> + if (IS_ERR(pci->dbi_base)) {
> + dev_err(dev, "couldn't remap dbi base %pR\n", dbi_res);
> + return PTR_ERR(pci->dbi_base);
> + }
> +
> + ecam_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config");
> + if (!ecam_res) {
> + dev_err(dev, "couldn't find 'config' reg in DT\n");
> + return -ENOENT;
> + }
> + al_pcie->ecam_size = resource_size(ecam_res);
> +
> + controller_res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> + "controller");
> + al_pcie->controller_base = devm_ioremap_resource(dev, controller_res);
> + if (IS_ERR(al_pcie->controller_base)) {
> + dev_err(dev, "couldn't remap controller base %pR\n",
> + controller_res);
> + return PTR_ERR(al_pcie->controller_base);
> + }
> +
> + dev_dbg(dev, "From DT: dbi_base: %pR, controller_base: %pR\n",
> + dbi_res, controller_res);
> +
> + platform_set_drvdata(pdev, al_pcie);
> +
> + ret = al_add_pcie_port(&pci->pp, pdev);
> +
> + return ret;
> +}
> +
> +static const struct of_device_id al_pcie_of_match[] = {
> + { .compatible = "amazon,al-pcie",
> + },
> + {},
> +};
> +
> +static struct platform_driver al_pcie_driver = {
> + .driver = {
> + .name = "al-pcie",
> + .of_match_table = al_pcie_of_match,
> + .suppress_bind_attrs = true,
> + },
> + .probe = al_pcie_probe,
> +};
> +builtin_platform_driver(al_pcie_driver);
> +
> +#endif /* CONFIG_PCIE_AL*/
> --
> 2.17.1
Reviewed-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
^ permalink raw reply
* RE: [PATCH v5 12/15] ARM: dts: imx6ul: add dma support on ecspi
From: Robin Gong @ 2019-07-23 9:39 UTC (permalink / raw)
To: Shawn Guo
Cc: robh+dt@kernel.org, mark.rutland@arm.com, s.hauer@pengutronix.de,
kernel@pengutronix.de, broonie@kernel.org, festevam@gmail.com,
vkoul@kernel.org, dan.j.williams@intel.com,
u.kleine-koenig@pengutronix.de, catalin.marinas@arm.com,
l.stach@pengutronix.de, will.deacon@arm.com,
linux-spi@vger.kernel.org, dl-linux-imx,
linux-arm-kernel@lists.infradead.org, linux-kern
In-Reply-To: <20190717064204.GA3738@dragon>
On 2019-7-17 at 14:42 Shawn Guo <shawnguo@kernel.org> wrote:
> On Mon, Jun 10, 2019 at 04:17:50PM +0800, yibin.gong@nxp.com wrote:
> > From: Robin Gong <yibin.gong@nxp.com>
> >
> > Add dma support on ecspi.
> >
> > Signed-off-by: Robin Gong <yibin.gong@nxp.com>
>
> Applied, thanks.
Thanks Shawn, but how about other dts patches such as 01/15,02/15?
Hi Mark and Vinod,
I got Ack from you last month on v5, https://patchwork.kernel.org/cover/10984301/
Is it the right time to apply v5 ? Any concern, please let me know, thanks.
^ permalink raw reply
* [PATCH v3 8/8] PCI: dw: Add support for PCI_PROBE_ONLY/PCI_REASSIGN_ALL_BUS flags
From: Jonathan Chocron @ 2019-07-23 9:27 UTC (permalink / raw)
To: lorenzo.pieralisi, bhelgaas, jingoohan1, gustavo.pimentel,
robh+dt, mark.rutland
Cc: dwmw, benh, alisaidi, ronenk, barakw, talel, hanochu, hhhawa,
linux-pci, linux-kernel, devicetree, jonnyc
In-Reply-To: <20190723092529.11310-1-jonnyc@amazon.com>
This basically aligns the usage of PCI_PROBE_ONLY and
PCI_REASSIGN_ALL_BUS in dw_pcie_host_init() with the logic in
pci_host_common_probe().
Now it will be possible to control via the devicetree whether to just
probe the PCI bus (in cases where FW already configured it) or to fully
configure it.
Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>
---
.../pci/controller/dwc/pcie-designware-host.c | 23 +++++++++++++++----
1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index d2ca748e4c85..0a294d8aa21a 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -342,6 +342,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
if (!bridge)
return -ENOMEM;
+ of_pci_check_probe_only();
+
ret = devm_of_pci_get_host_bridge_resources(dev, 0, 0xff,
&bridge->windows, &pp->io_base);
if (ret)
@@ -474,6 +476,10 @@ int dw_pcie_host_init(struct pcie_port *pp)
pp->root_bus_nr = pp->busn->start;
+ /* Do not reassign bus nums if probe only */
+ if (!pci_has_flag(PCI_PROBE_ONLY))
+ pci_add_flags(PCI_REASSIGN_ALL_BUS);
+
bridge->dev.parent = dev;
bridge->sysdata = pp;
bridge->busnr = pp->root_bus_nr;
@@ -490,11 +496,20 @@ int dw_pcie_host_init(struct pcie_port *pp)
if (pp->ops->scan_bus)
pp->ops->scan_bus(pp);
- pci_bus_size_bridges(pp->root_bus);
- pci_bus_assign_resources(pp->root_bus);
+ /*
+ * We insert PCI resources into the iomem_resource and
+ * ioport_resource trees in either pci_bus_claim_resources()
+ * or pci_bus_assign_resources().
+ */
+ if (pci_has_flag(PCI_PROBE_ONLY)) {
+ pci_bus_claim_resources(pp->root_bus);
+ } else {
+ pci_bus_size_bridges(pp->root_bus);
+ pci_bus_assign_resources(pp->root_bus);
- list_for_each_entry(child, &pp->root_bus->children, node)
- pcie_bus_configure_settings(child);
+ list_for_each_entry(child, &pp->root_bus->children, node)
+ pcie_bus_configure_settings(child);
+ }
pci_bus_add_devices(pp->root_bus);
return 0;
--
2.17.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