* Re: [PATCH V4 04/10] PM / OPP: Manage supply's voltage/current in a separate structure
From: Stephen Boyd @ 2016-11-29 0:58 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael Wysocki, nm, Viresh Kumar, linaro-kernel, linux-pm,
linux-kernel, Vincent Guittot, robh, d-gerlach, broonie,
devicetree
In-Reply-To: <6078de802f62a50e4da80022949745fc9f0bdce8.1479986491.git.viresh.kumar@linaro.org>
On 11/24, Viresh Kumar wrote:
> This is a preparatory step for multiple regulator per device support.
> Move the voltage/current variables to a new structure.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> Tested-by: Dave Gerlach <d-gerlach@ti.com>
> ---
Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH V4 05/10] PM / OPP: Pass struct dev_pm_opp_supply to _set_opp_voltage()
From: Stephen Boyd @ 2016-11-29 0:59 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael Wysocki, nm-l0cyMroinI0, Viresh Kumar,
linaro-kernel-cunTk1MwBs8s++Sfvej+rw,
linux-pm-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Vincent Guittot,
robh-DgEjT+Ai2ygdnm+yROfE0A, d-gerlach-l0cyMroinI0,
broonie-DgEjT+Ai2ygdnm+yROfE0A, devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <d71095ecb8e8dde5861e73f2b8df8955f2e2fb9c.1479986491.git.viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
On 11/24, Viresh Kumar wrote:
> Pass the entire supply structure instead of all of its fields.
>
> Signed-off-by: Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Tested-by: Dave Gerlach <d-gerlach-l0cyMroinI0@public.gmane.org>
> ---
Reviewed-by: Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH V2 03/10] Documentation: devicetree: thermal: da9062/61 TJUNC temperature binding
From: Eduardo Valentin @ 2016-11-29 0:59 UTC (permalink / raw)
To: Steve Twiss
Cc: DEVICETREE, LINUX-KERNEL, LINUX-PM, Mark Rutland, Rob Herring,
Zhang Rui, Dmitry Torokhov, Guenter Roeck, LINUX-INPUT,
LINUX-WATCHDOG, Lee Jones, Liam Girdwood, Mark Brown,
Support Opensource, Wim Van Sebroeck
In-Reply-To: <d8c986e39bafc2b8a1c5c23e722f61cdd021e9be.1477501000.git.stwiss.opensource-WBD+wuPFNBhBDgjK7y7TUQ@public.gmane.org>
On Wed, Oct 26, 2016 at 05:56:37PM +0100, Steve Twiss wrote:
> From: Steve Twiss <stwiss.opensource-WBD+wuPFNBhBDgjK7y7TUQ@public.gmane.org>
>
> Device tree binding information for DA9062 and DA9061 thermal junction
> temperature monitor.
>
> Binding descriptions for the DA9061 and DA9062 thermal TJUNC supervisor
> device driver, using a single THERMAL_TRIP_HOT trip-wire and allowing for
> a configurable polling period for over-temperature polling.
>
> Signed-off-by: Steve Twiss <stwiss.opensource-WBD+wuPFNBhBDgjK7y7TUQ@public.gmane.org>
>
> ---
> This patch applies against linux-next and v4.8
>
> v1 -> v2
> - Patch renamed from [PATCH V1 08/10] to [PATCH V2 03/10] -- these
> changes were made to fix checkpatch warnings caused by the patch
> set dependency order
> - A second example for DA9061 is provided to highlight the use of a
> fall-back compatible option for the DA9062 watchdog driver
>
> Hi,
>
> This patch depends on acceptance of the main code for the thermal device
> driver:
> [PATCH V2 09/10] thermal: da9061: TJUNC temperature driver
>
> The previous [PATCH V1 08/10] was acked-by: Rob Herring, however this has
> not been added because changes have been made to add a new binding
> example. This describes the use of DA9061.
> This addition was made after alterations to the device driver meant that a
> fall-back compatible string could reuse the DA9062 device driver.
>
> Regards,
> Steve Twiss, Dialog Semiconductor Ltd.
>
>
> .../devicetree/bindings/thermal/da9062-thermal.txt | 37 ++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/thermal/da9062-thermal.txt
>
> diff --git a/Documentation/devicetree/bindings/thermal/da9062-thermal.txt b/Documentation/devicetree/bindings/thermal/da9062-thermal.txt
> new file mode 100644
> index 0000000..fb207ff
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/thermal/da9062-thermal.txt
> @@ -0,0 +1,37 @@
> +* Dialog DA9062/61 TJUNC Thermal Module
> +
> +This module is part of the DA9061/DA9062. For more details about entire
> +DA9062 and DA9061 chips see Documentation/devicetree/bindings/mfd/da9062.txt
> +
> +Junction temperature thermal module uses an interrupt signal to identify
> +high THERMAL_TRIP_HOT temperatures for the PMIC device.
> +
> +Required properties:
> +
> +- compatible: should be one of:
> + dlg,da9061-thermal
> + dlg,da9062-thermal
> +
> +Optional properties:
> +
> +- dlg,tjunc-temp-polling-period-ms : Specify the polling period, measured
> + in milliseconds, between thermal zone device update checks.
Can you please elaborate on why you need this chip manufacture specific property?
Can we use the polling property of already existing in the
Documentation/devicetree/bindings/thermal/thermal.txt
See the polling properties.
> +
> +Example: DA9061
> +
> + pmic0: da9062@58 {
> + thermal {
> + compatible = "dlg,da9062-thermal";
> + dlg,tjunc-temp-polling-period-ms = <3000>;
Same
> + };
> + };
> +
> +Example: DA9061 using a fall-back compatible for the DA9062 onkey driver
> +
> + pmic0: da9061@58 {
> + thermal {
> + compatible = "dlg,da9061-thermal", "dlg,da9062-thermal";
> + dlg,tjunc-temp-polling-period-ms = <3000>;
Same
> + };
> + };
> +
> --
> end-of-patch for PATCH V2
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH V4 07/10] PM / OPP: Separate out _generic_opp_set_rate()
From: Stephen Boyd @ 2016-11-29 1:09 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael Wysocki, nm-l0cyMroinI0, Viresh Kumar,
linaro-kernel-cunTk1MwBs8s++Sfvej+rw,
linux-pm-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Vincent Guittot,
robh-DgEjT+Ai2ygdnm+yROfE0A, d-gerlach-l0cyMroinI0,
broonie-DgEjT+Ai2ygdnm+yROfE0A, devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1f8634c64ccec1d24119afea4b6bd2d7d1a911cf.1479986492.git.viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
On 11/24, Viresh Kumar wrote:
> Later patches would add support for custom opp_set_rate callbacks. This
> patch separates out the code for generic opp_set_rate handler in order
> to prepare for that.
s/opp_set_rate/set_opp/ twice?
>
> Signed-off-by: Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Tested-by: Dave Gerlach <d-gerlach-l0cyMroinI0@public.gmane.org>
> ---
Besides the naming confusion.
Reviewed-by: Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> @@ -1422,6 +1488,11 @@ int dev_pm_opp_set_regulators(struct device *dev, const char * const names[],
>
> opp_table->regulator_count = count;
>
> + /* Allocate block only once to pass to ->set_rate() */
_generic_set_opp()? Or just set_opp when that gets introduced in
the next patch.
> + ret = _allocate_set_opp_data(opp_table);
> + if (ret)
> + goto free_regulators;
> +
> mutex_unlock(&opp_table_lock);
> return 0;
>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH V4 08/10] PM / OPP: Allow platform specific custom set_opp() callbacks
From: Stephen Boyd @ 2016-11-29 1:16 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael Wysocki, nm, Viresh Kumar, linaro-kernel, linux-pm,
linux-kernel, Vincent Guittot, robh, d-gerlach, broonie,
devicetree
In-Reply-To: <743654b6d2b18f94cd36b8b700c028f67cebaada.1479986492.git.viresh.kumar@linaro.org>
On 11/24, Viresh Kumar wrote:
> diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
> index 99491f4099e5..7c945d5950bf 100644
> --- a/drivers/base/power/opp/core.c
> +++ b/drivers/base/power/opp/core.c
> @@ -673,6 +673,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
> {
> struct opp_table *opp_table;
> unsigned long freq, old_freq;
> + int (*set_opp)(struct device *dev, struct dev_pm_set_opp_data *data);
Curious why dev is an argument that isn't part of struct
dev_pm_set_opp_data here? Is there some benefit to keeping it
split out?
> struct dev_pm_opp *old_opp, *opp;
> struct regulator **regulators;
> struct dev_pm_set_opp_data *data;
> @@ -1488,7 +1497,7 @@ int dev_pm_opp_set_regulators(struct device *dev, const char * const names[],
>
> opp_table->regulator_count = count;
>
> - /* Allocate block only once to pass to ->set_rate() */
> + /* Allocate block only once to pass to ->set_opp() */
Ah here it is.
> ret = _allocate_set_opp_data(opp_table);
> if (ret)
> goto free_regulators;
> @@ -1563,6 +1572,109 @@ void dev_pm_opp_put_regulators(struct device *dev)
> EXPORT_SYMBOL_GPL(dev_pm_opp_put_regulators);
>
> /**
> + * dev_pm_opp_register_set_opp_helper() - Register custom OPP set rate helper
s/custom OPP set rate/custom set OPP/ ?
> + * @dev: Device for which the helper is getting registered.
> + * @set_opp: Custom set OPP helper.
> + *
> + * This is useful to support complex platforms (like platforms with multiple
> + * regulators per device), instead of the generic OPP set rate helper.
> + *
> + * This must be called before any OPPs are initialized for the device.
> + *
> + * Locking: The internal opp_table and opp structures are RCU protected.
> + * Hence this function internally uses RCU updater strategy with mutex locks
> + * to keep the integrity of the internal data structures. Callers should ensure
> + * that this function is *NOT* called under RCU protection or in contexts where
> + * mutex cannot be locked.
> + */
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH V4 10/10] PM / OPP: Don't assume platform doesn't have regulators
From: Stephen Boyd @ 2016-11-29 1:18 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael Wysocki, nm-l0cyMroinI0, Viresh Kumar,
linaro-kernel-cunTk1MwBs8s++Sfvej+rw,
linux-pm-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Vincent Guittot,
robh-DgEjT+Ai2ygdnm+yROfE0A, d-gerlach-l0cyMroinI0,
broonie-DgEjT+Ai2ygdnm+yROfE0A, devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <82cc855c7abb5d0583abe4cc9132c7e589434814.1479986492.git.viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
On 11/24, Viresh Kumar wrote:
> If the regulators aren't set explicitly by the platform, the OPP core
> assumes that the platform doesn't have any regulator and uses the
> clk-only callback.
>
> If the platform failed to register a regulator with the core, then this
> can turn out to be a dangerous assumption as the OPP core will try to
> change clk without changing regulators.
>
> Handle that properly by making sure that the DT didn't have any entries
> for supply voltages as well.
>
> Signed-off-by: Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
Reviewed-by: Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v4] clkdev: add devm_of_clk_get()
From: Kuninori Morimoto @ 2016-11-29 1:21 UTC (permalink / raw)
To: Russell King - ARM Linux, Stephen Boyd, Rob Herring, Linux-ALSA,
Linux-DT, Michael Turquette, Linux-Kernel, Mark Brown,
linux-clk-u79uwXL29TY76Z2rM5mHXA, Linux-ARM
From: Kuninori Morimoto <kuninori.morimoto.gx-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
Current Linux has of_clk_get(), but doesn't have devm_of_clk_get().
This patch adds it. It is implemeted in clk-devres.c to share
devm_clk_release().
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
---
v3 -> v4
- git log explain why it is implemeted in clk-devres
- it is related to CONFIG_HAVE_CLK
drivers/clk/clk-devres.c | 21 +++++++++++++++++++++
include/linux/clk.h | 27 +++++++++++++++++++++++----
2 files changed, 44 insertions(+), 4 deletions(-)
diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index 8f57154..2449b25 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -53,3 +53,24 @@ void devm_clk_put(struct device *dev, struct clk *clk)
WARN_ON(ret);
}
EXPORT_SYMBOL(devm_clk_put);
+
+struct clk *devm_of_clk_get(struct device *dev,
+ struct device_node *np, int index)
+{
+ struct clk **ptr, *clk;
+
+ ptr = devres_alloc(devm_clk_release, sizeof(*ptr), GFP_KERNEL);
+ if (!ptr)
+ return ERR_PTR(-ENOMEM);
+
+ clk = of_clk_get(np, index);
+ if (!IS_ERR(clk)) {
+ *ptr = clk;
+ devres_add(dev, ptr);
+ } else {
+ devres_free(ptr);
+ }
+
+ return clk;
+}
+EXPORT_SYMBOL(devm_of_clk_get);
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 123c027..7f50c5f 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -17,8 +17,9 @@
#include <linux/notifier.h>
struct device;
-
struct clk;
+struct device_node;
+struct of_phandle_args;
/**
* DOC: clk notifier callback types
@@ -249,6 +250,21 @@ static inline void clk_unprepare(struct clk *clk)
struct clk *devm_clk_get(struct device *dev, const char *id);
/**
+ * devm_clk_get - lookup and obtain a managed reference to a clock producer.
+ * @dev: device for clock "consumer"
+ * @np: pointer to clock consumer node
+ * @index: clock index
+ *
+ * This function parses the clocks, and uses them to look up the
+ * struct clk from the registered list of clock providers by using
+ * @np and @index.
+ *
+ * The clock will automatically be freed when the device is unbound
+ * from the bus.
+ */
+struct clk *devm_of_clk_get(struct device *dev, struct device_node *np, int index);
+
+/**
* clk_enable - inform the system when the clock source should be running.
* @clk: clock source
*
@@ -432,6 +448,12 @@ static inline struct clk *devm_clk_get(struct device *dev, const char *id)
return NULL;
}
+static inline struct clk *devm_of_clk_get(struct device *dev,
+ struct device_node *np, int index)
+{
+ return NULL;
+}
+
static inline void clk_put(struct clk *clk) {}
static inline void devm_clk_put(struct device *dev, struct clk *clk) {}
@@ -501,9 +523,6 @@ static inline void clk_disable_unprepare(struct clk *clk)
clk_unprepare(clk);
}
-struct device_node;
-struct of_phandle_args;
-
#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
struct clk *of_clk_get(struct device_node *np, int index);
struct clk *of_clk_get_by_name(struct device_node *np, const char *name);
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH V2 09/10] thermal: da9062/61: Thermal junction temperature monitoring driver
From: Eduardo Valentin @ 2016-11-29 1:24 UTC (permalink / raw)
To: Steve Twiss
Cc: LINUX-KERNEL, LINUX-PM, Zhang Rui, DEVICETREE, Dmitry Torokhov,
Guenter Roeck, LINUX-INPUT, LINUX-WATCHDOG, Lee Jones,
Liam Girdwood, Mark Brown, Mark Rutland, Rob Herring,
Support Opensource, Wim Van Sebroeck
In-Reply-To: <20fefc922818ab0511101f73b1a4d3468c9a8ed3.1477501000.git.stwiss.opensource-WBD+wuPFNBhBDgjK7y7TUQ@public.gmane.org>
Hello Steve,
On Wed, Oct 26, 2016 at 05:56:39PM +0100, Steve Twiss wrote:
> From: Steve Twiss <stwiss.opensource-WBD+wuPFNBhBDgjK7y7TUQ@public.gmane.org>
>
> Add junction temperature monitoring supervisor device driver, compatible
> with the DA9062 and DA9061 PMICs.
>
> If the PMIC's internal junction temperature rises above TEMP_WARN (125
> degC) an interrupt is issued. This TEMP_WARN level is defined as the
> THERMAL_TRIP_HOT trip-wire inside the device driver.
>
> The thermal triggering mechanism is interrupt based and happens when the
> temperature rises above a given threshold level. The component cannot
> return an exact temperature, it only has knowledge if the temperature is
> above or below a given threshold value. A status bit must be polled to
> detect when the temperature falls below that threshold level again. A
> kernel work queue is configured to repeatedly poll and detect when the
> temperature falls below this trip-wire, between 1 and 10 second intervals
> (defaulting at 3 seconds).
>
> This first level of temperature supervision is intended for non-invasive
> temperature control, where the necessary measures for cooling the system
> down are left to the host software.
>
> Signed-off-by: Steve Twiss <stwiss.opensource-WBD+wuPFNBhBDgjK7y7TUQ@public.gmane.org>
>
> ---
> This patch applies against linux-next and v4.8
>
> v1 -> v2
> - Patch renamed from [PATCH V1 05/10] to [PATCH V2 09/10] -- these
> changes were made to fix checkpatch warnings caused by the patch
> set dependency order
> - List the header files in alphabetical order
> - Remove "GPL v2" and replace with MODULE_LICENSE("GPL") to match the
> copyright "GNU Public License v2 or later" option in the header
> comment for this file. See the allowed identifiers in the file
> include/linux/module.h +170
> - Remove notify function "da9062_thermal_notify" function.
> - MODULE_AUTHOR() macros removes Company Name and just gives Name in
> accordance with include/linux/module.h +200
> - Remove the compatible "dlg,da9061-thermal" option in the of_device_id
> struct table. This patch now assumes the use of a DA9062 fallback
> compatible string in the DTS when using the DA9061 thermal component
> of the DA9061 device.
> - Re-ordered some assignments earlier in the probe() for thermal->hw,
> thermal->polling_period, thermal->mode, thermal->dev
> - Added further information in the patch description to explain the use
> of the device driver's built-in work-queue.
>
> Regards,
> Steve Twiss, Dialog Semiconductor Ltd.
>
>
> drivers/thermal/Kconfig | 10 ++
> drivers/thermal/Makefile | 1 +
> drivers/thermal/da9062-thermal.c | 289 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 300 insertions(+)
> create mode 100644 drivers/thermal/da9062-thermal.c
>
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 2d702ca..da58e54 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -272,6 +272,16 @@ config DB8500_CPUFREQ_COOLING
> bound cpufreq cooling device turns active to set CPU frequency low to
> cool down the CPU.
>
> +config DA9062_THERMAL
> + tristate "DA9062/DA9061 Dialog Semiconductor thermal driver"
> + depends on MFD_DA9062
> + depends on OF
> + help
> + Enable this for the Dialog Semiconductor thermal sensor driver.
> + This will report PMIC junction over-temperature for one thermal trip
> + zone.
> + Compatible with the DA9062 and DA9061 PMICs.
Is there any hardware documentation available for this chip that can be
pointed out?
> +
> config INTEL_POWERCLAMP
> tristate "Intel PowerClamp idle injection driver"
> depends on THERMAL
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 10b07c1..0a2b3f2 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -38,6 +38,7 @@ obj-$(CONFIG_ARMADA_THERMAL) += armada_thermal.o
> obj-$(CONFIG_TANGO_THERMAL) += tango_thermal.o
> obj-$(CONFIG_IMX_THERMAL) += imx_thermal.o
> obj-$(CONFIG_DB8500_CPUFREQ_COOLING) += db8500_cpufreq_cooling.o
> +obj-$(CONFIG_DA9062_THERMAL) += da9062-thermal.o
> obj-$(CONFIG_INTEL_POWERCLAMP) += intel_powerclamp.o
> obj-$(CONFIG_X86_PKG_TEMP_THERMAL) += x86_pkg_temp_thermal.o
> obj-$(CONFIG_INTEL_SOC_DTS_IOSF_CORE) += intel_soc_dts_iosf.o
> diff --git a/drivers/thermal/da9062-thermal.c b/drivers/thermal/da9062-thermal.c
> new file mode 100644
> index 0000000..e0d2b1b
> --- /dev/null
> +++ b/drivers/thermal/da9062-thermal.c
> @@ -0,0 +1,289 @@
> +/*
> + * Thermal device driver for DA9062 and DA9061
> + * Copyright (C) 2016 Dialog Semiconductor Ltd.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/thermal.h>
> +#include <linux/workqueue.h>
> +
> +#include <linux/mfd/da9062/core.h>
> +#include <linux/mfd/da9062/registers.h>
> +
> +#define DA9062_DEFAULT_POLLING_MS_PERIOD 3000
> +#define DA9062_MAX_POLLING_MS_PERIOD 10000
> +#define DA9062_MIN_POLLING_MS_PERIOD 1000
> +
> +#define DA9062_MILLI_CELSIUS(t) ((t)*1000)
> +
> +struct da9062_thermal_config {
> + const char *name;
> +};
> +
> +struct da9062_thermal {
> + struct da9062 *hw;
> + struct delayed_work work;
> + struct thermal_zone_device *zone;
> + enum thermal_device_mode mode;
> + unsigned int polling_period;
> + struct mutex lock;
> + int temperature;
> + int irq;
> + const struct da9062_thermal_config *config;
> + struct device *dev;
> +};
> +
> +static void da9062_thermal_poll_on(struct work_struct *work)
> +{
> + struct da9062_thermal *thermal = container_of(work,
> + struct da9062_thermal,
> + work.work);
> + unsigned int val;
> + int ret;
> +
> + /* clear E_TEMP */
> + ret = regmap_write(thermal->hw->regmap,
> + DA9062AA_EVENT_B,
> + DA9062AA_E_TEMP_MASK);
> + if (ret < 0) {
> + dev_err(thermal->dev,
> + "Cannot clear the TJUNC temperature status\n");
> + goto err_enable_irq;
> + }
> +
> + /* Now read E_TEMP again: it is acting like a status bit.
> + * If over-temperature, then this status will be true.
> + * If not over-temperature, this status will be false.
> + */
> + ret = regmap_read(thermal->hw->regmap,
> + DA9062AA_EVENT_B,
> + &val);
> + if (ret < 0) {
> + dev_err(thermal->dev,
> + "Cannot check the TJUNC temperature status\n");
> + goto err_enable_irq;
> + } else {
> + if (val & DA9062AA_E_TEMP_MASK) {
> + mutex_lock(&thermal->lock);
> + thermal->temperature = DA9062_MILLI_CELSIUS(125);
Does this mean that the chip temperature is above or equal to 125C, is
this really a safe temperature to keep it running?
> + mutex_unlock(&thermal->lock);
> + thermal_zone_device_update(thermal->zone);
> +
> + schedule_delayed_work(&thermal->work,
> + msecs_to_jiffies(thermal->polling_period));
> + return;
> + } else {
> + mutex_lock(&thermal->lock);
> + thermal->temperature = DA9062_MILLI_CELSIUS(0);
> + mutex_unlock(&thermal->lock);
> + thermal_zone_device_update(thermal->zone);
> + }
> + }
> +
> +err_enable_irq:
> + enable_irq(thermal->irq);
> +}
> +
> +static irqreturn_t da9062_thermal_irq_handler(int irq, void *data)
> +{
> + struct da9062_thermal *thermal = data;
> +
> + disable_irq_nosync(thermal->irq);
> + schedule_delayed_work(&thermal->work, 0);
Can you please elaborate a little on why you have an one shot threaded IRQ
handler that is only disabling the IRQ then, scheduling a work with delay of 0?
To my understanding, this is exactly what you get when you run your
threaded IRQ handler, when you configure it as one shot.
Have you tried simply handling the same work done in your workqueue
handler in your threaded IRQ? That should simplify your code and get the
same result you are expecting.
If you are not getting the IRQ disabled on the threaded IRQ when
configured as one shot, something else seams to be broken.
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int da9062_thermal_get_mode(struct thermal_zone_device *z,
> + enum thermal_device_mode *mode)
> +{
> + struct da9062_thermal *thermal = z->devdata;
> + *mode = thermal->mode;
> + return 0;
> +}
> +
> +static int da9062_thermal_get_trip_type(struct thermal_zone_device *z,
> + int trip,
> + enum thermal_trip_type *type)
> +{
> + struct da9062_thermal *thermal = z->devdata;
> +
> + switch (trip) {
> + case 0:
> + *type = THERMAL_TRIP_HOT;
> + break;
> + default:
> + dev_err(thermal->dev,
> + "Driver does not support more than 1 trip-wire\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int da9062_thermal_get_trip_temp(struct thermal_zone_device *z,
> + int trip,
> + int *temp)
> +{
> + struct da9062_thermal *thermal = z->devdata;
> +
> + switch (trip) {
> + case 0:
> + *temp = DA9062_MILLI_CELSIUS(125);
> + break;
> + default:
> + dev_err(thermal->dev,
> + "Driver does not support more than 1 trip-wire\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int da9062_thermal_get_temp(struct thermal_zone_device *z,
> + int *temp)
> +{
> + struct da9062_thermal *thermal = z->devdata;
> +
> + mutex_lock(&thermal->lock);
> + *temp = thermal->temperature;
> + mutex_unlock(&thermal->lock);
> +
> + return 0;
> +}
> +
> +static struct thermal_zone_device_ops da9062_thermal_ops = {
> + .get_temp = da9062_thermal_get_temp,
> + .get_mode = da9062_thermal_get_mode,
> + .get_trip_type = da9062_thermal_get_trip_type,
> + .get_trip_temp = da9062_thermal_get_trip_temp,
> +};
> +
> +static const struct da9062_thermal_config da9062_config = {
> + .name = "da9062-thermal",
> +};
> +
> +static const struct of_device_id da9062_compatible_reg_id_table[] = {
> + { .compatible = "dlg,da9062-thermal", .data = &da9062_config },
> + { },
> +};
> +
> +static int da9062_thermal_probe(struct platform_device *pdev)
> +{
> + struct da9062 *chip = dev_get_drvdata(pdev->dev.parent);
> + struct da9062_thermal *thermal;
> + unsigned int pp_tmp = DA9062_DEFAULT_POLLING_MS_PERIOD;
> + const struct of_device_id *match;
> + int ret = 0;
> +
> + match = of_match_node(da9062_compatible_reg_id_table,
> + pdev->dev.of_node);
> + if (!match)
> + return -ENXIO;
> +
> + if (pdev->dev.of_node) {
> + if (!of_property_read_u32(pdev->dev.of_node,
> + "dlg,tjunc-temp-polling-period-ms",
> + &pp_tmp)) {
> + if (pp_tmp < DA9062_MIN_POLLING_MS_PERIOD ||
> + pp_tmp > DA9062_MAX_POLLING_MS_PERIOD)
> + pp_tmp = DA9062_DEFAULT_POLLING_MS_PERIOD;
> + }
> +
> + dev_dbg(&pdev->dev,
> + "TJUNC temp polling period set at %d ms\n",
> + pp_tmp);
> + }
> +
> + thermal = devm_kzalloc(&pdev->dev, sizeof(struct da9062_thermal),
> + GFP_KERNEL);
> + if (!thermal) {
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + thermal->config = match->data;
> + thermal->hw = chip;
> + thermal->polling_period = pp_tmp;
> + thermal->mode = THERMAL_DEVICE_ENABLED;
> + thermal->dev = &pdev->dev;
> +
> + INIT_DELAYED_WORK(&thermal->work, da9062_thermal_poll_on);
> + mutex_init(&thermal->lock);
> +
> + thermal->zone = thermal_zone_device_register(thermal->config->name,
> + 1, 0, thermal,
> + &da9062_thermal_ops, NULL, 0,
> + 0);
Did you try using of-thermal?
So you would avoid having specific DT properties for something that
there is already a defined property?
> + if (IS_ERR(thermal->zone)) {
> + dev_err(&pdev->dev, "Cannot register thermal zone device\n");
> + ret = PTR_ERR(thermal->zone);
> + goto err;
> + }
> +
> + ret = platform_get_irq_byname(pdev, "THERMAL");
> + if (ret < 0) {
> + dev_err(&pdev->dev, "Failed to get platform IRQ.\n");
> + goto err_zone;
> + }
> + thermal->irq = ret;
> +
> + ret = request_threaded_irq(thermal->irq, NULL,
> + da9062_thermal_irq_handler,
> + IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> + "THERMAL", thermal);
How about using the devm_ version?
> + if (ret) {
> + dev_err(&pdev->dev,
> + "Failed to request thermal device IRQ.\n");
> + goto err_zone;
> + }
> +
> + platform_set_drvdata(pdev, thermal);
> + return 0;
> +
> +err_zone:
> + thermal_zone_device_unregister(thermal->zone);
> +err:
> + return ret;
> +}
> +
> +static int da9062_thermal_remove(struct platform_device *pdev)
> +{
> + struct da9062_thermal *thermal = platform_get_drvdata(pdev);
> +
> + free_irq(thermal->irq, thermal);
> + thermal_zone_device_unregister(thermal->zone);
> + cancel_delayed_work_sync(&thermal->work);
> + return 0;
> +}
> +
> +static struct platform_driver da9062_thermal_driver = {
> + .probe = da9062_thermal_probe,
> + .remove = da9062_thermal_remove,
> + .driver = {
> + .name = "da9062-thermal",
> + .of_match_table = da9062_compatible_reg_id_table,
> + },
> +};
> +
> +module_platform_driver(da9062_thermal_driver);
> +
> +MODULE_AUTHOR("Steve Twiss");
> +MODULE_DESCRIPTION("Thermal TJUNC device driver for Dialog DA9062 and DA9061");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:da9062-thermal");
> --
> end-of-patch for PATCH V2
>
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH V8 2/6] thermal: bcm2835: add thermal driver for bcm2835 soc
From: Eduardo Valentin @ 2016-11-29 1:34 UTC (permalink / raw)
To: Eric Anholt
Cc: Martin Sperl, Zhang Rui, Rob Herring, Pawel Moll, Mark Rutland,
Stephen Warren, Lee Jones, Russell King, Florian Fainelli,
Catalin Marinas, Will Deacon, linux-pm, devicetree,
linux-rpi-kernel, linux-arm-kernel
In-Reply-To: <87twarz6s1.fsf@eliezer.anholt.net>
Hello Eric, Martin,
On Mon, Nov 28, 2016 at 12:30:38PM -0800, Eric Anholt wrote:
> Eduardo Valentin <edubezval@gmail.com> writes:
>
<cut>
> The firmware today always initializes thermal. I suggested adding the
> init code because we (myself and the Pi Foundation) would like to reduce
> how much closed firmware code is required in the platform, and the Linux
> driver doing this would help make that possible in the future.
Oh I see. Backup code for future chips/firmware.
>
> >> > Who has the ownership of this device?
> >>
> >> Joined ownership I suppose...
> >>
> >
> > with no synchronization mechanism?
>
> Correct, because none is necessary.
>
<cut>
>
> Either the device was initialized by the firmware before handing off to
> ARM (today's firmware) or it never will be (potential future firmware).
And do you have a way to check if the firmware has the initialization
code or not? By firmware version, for example. Or even, chip version,
maybe?
If the current firmware will always initialize the chip, I would say,
ARM should simply read the registers, no initialization, unless it is
known that the firmware intentionally left the device uninitialized.
Again, just trying to avoid obscure misbehavior, when running into a
faulty state, and running silently broken.
^ permalink raw reply
* Re: [PATCH v10 2/4] dtc: Document the dynamic plugin internals
From: Frank Rowand @ 2016-11-29 1:36 UTC (permalink / raw)
To: Pantelis Antoniou, David Gibson
Cc: Jon Loeliger, Grant Likely, Rob Herring, Jan Luebbe, Sascha Hauer,
Phil Elwell, Simon Glass, Maxime Ripard, Thomas Petazzoni,
Boris Brezillon, Antoine Tenart, Stephen Boyd,
Devicetree Compiler, devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1480077131-14526-3-git-send-email-pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
On 11/25/16 04:32, Pantelis Antoniou wrote:
> Provides the document explaining the internal mechanics of
> plugins and options.
>
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> ---
> Documentation/dt-object-internal.txt | 318 +++++++++++++++++++++++++++++++++++
> 1 file changed, 318 insertions(+)
> create mode 100644 Documentation/dt-object-internal.txt
>
> diff --git a/Documentation/dt-object-internal.txt b/Documentation/dt-object-internal.txt
> new file mode 100644
> index 0000000..d5b841e
> --- /dev/null
> +++ b/Documentation/dt-object-internal.txt
> @@ -0,0 +1,318 @@
> +Device Tree Dynamic Object format internals
> +-------------------------------------------
> +
> +The Device Tree for most platforms is a static representation of
> +the hardware capabilities. This is insufficient for many platforms
> +that need to dynamically insert device tree fragments to the
> +running kernel's live tree.
> +
> +This document explains the the device tree object format and the
> +modifications made to the device tree compiler, which make it possible.
> +
> +1. Simplified Problem Definition
> +--------------------------------
> +
> +Assume we have a platform which boots using following simplified device tree.
> +
> +---- foo.dts -----------------------------------------------------------------
> + /* FOO platform */
> + / {
> + compatible = "corp,foo";
> +
> + /* shared resources */
> + res: res {
> + };
> +
> + /* On chip peripherals */
> + ocp: ocp {
> + /* peripherals that are always instantiated */
> + peripheral1 { ... };
> + };
> + };
> +---- foo.dts -----------------------------------------------------------------
> +
> +We have a number of peripherals that after probing (using some undefined method)
> +should result in different device tree configuration.
> +
> +We cannot boot with this static tree because due to the configuration of the
> +foo platform there exist multiple conficting peripherals DT fragments.
^^^^^^^^^^ conflicting
I assume conflicting because, for instance, the different peripherals might
occupy the same address space, use the same interrupt, or use the same gpio.
Mentioning that would provide a fuller picture for the neophyte.
> +
> +So for the bar peripheral we would have this:
> +
> +---- foo+bar.dts -------------------------------------------------------------
> + /* FOO platform + bar peripheral */
> + / {
> + compatible = "corp,foo";
> +
> + /* shared resources */
> + res: res {
> + };
> +
> + /* On chip peripherals */
> + ocp: ocp {
> + /* peripherals that are always instantiated */
> + peripheral1 { ... };
> +
> + /* bar peripheral */
> + bar {
> + compatible = "corp,bar";
> + ... /* various properties and child nodes */
> + };
> + };
> + };
> +---- foo+bar.dts -------------------------------------------------------------
> +
> +While for the baz peripheral we would have this:
> +
> +---- foo+baz.dts -------------------------------------------------------------
> + /* FOO platform + baz peripheral */
> + / {
> + compatible = "corp,foo";
> +
> + /* shared resources */
> + res: res {
> + /* baz resources */
> + baz_res: res_baz { ... };
> + };
> +
> + /* On chip peripherals */
> + ocp: ocp {
> + /* peripherals that are always instantiated */
> + peripheral1 { ... };
> +
> + /* baz peripheral */
> + baz {
> + compatible = "corp,baz";
> + /* reference to another point in the tree */
> + ref-to-res = <&baz_res>;
> + ... /* various properties and child nodes */
> + };
> + };
> + };
> +---- foo+baz.dts -------------------------------------------------------------
> +
> +We note that the baz case is more complicated, since the baz peripheral needs to
> +reference another node in the DT tree.
I know that there are other situations that can justify overlays, so not
contesting the basic need with this comment. But the above situation could
be handled in a much simpler fashion by setting the status property of each
of the conflicting devices to disabled, then after probing setting the status
to ok. That method removes a lot of complexity.
A big driver for the concept of overlays was being able to describe different
add on boards at run time, instead of when the base dtb was created. I think
we have agreed that moving to a connector model instead of a raw overlay is
the proper way to address add on boards.
Can you address how an overlay can be created that will work for a board
plugged into any of the identical sockets that is compatible with the
board?
> +
> +2. Device Tree Object Format Requirements
> +-----------------------------------------
> +
> +Since the device tree is used for booting a number of very different hardware
> +platforms it is imperative that we tread very carefully.
> +
> +2.a) No changes to the Device Tree binary format for the base tree. We cannot
> +modify the tree format at all and all the information we require should be
> +encoded using device tree itself. We can add nodes that can be safely ignored
> +by both bootloaders and the kernel. The plugin dtb's are optionally tagged
> +with a different magic number in the header but otherwise they too are simple
> +blobs.
> +
> +2.b) Changes to the DTS source format should be absolutely minimal, and should
> +only be needed for the DT fragment definitions, and not the base boot DT.
> +
> +2.c) An explicit option should be used to instruct DTC to generate the required
> +information needed for object resolution. Platforms that don't use the
> +dynamic object format can safely ignore it.
> +
> +2.d) Finally, DT syntax changes should be kept to a minimum. It should be
> +possible to express everything using the existing DT syntax.
> +
> +3. Implementation
> +-----------------
> +
> +The basic unit of addressing in Device Tree is the phandle. Turns out it's
> +relatively simple to extend the way phandles are generated and referenced
> +so that it's possible to dynamically convert symbolic references (labels)
> +to phandle values. This is a valid assumption as long as the author uses
> +reference syntax and does not assign phandle values manually (which might
> +be a problem with decompiled source files).
> +
> +We can roughly divide the operation into two steps.
> +
> +3.a) Compilation of the base board DTS file using the '-@' option
> +generates a valid DT blob with an added __symbols__ node at the root node,
> +containing a list of all nodes that are marked with a label.
> +
> +Using the foo.dts file above the following node will be generated;
> +
> +$ dtc -@ -O dtb -o foo.dtb -b 0 foo.dts
> +$ fdtdump foo.dtb
> +...
> +/ {
> + ...
> + res {
> + ...
> + phandle = <0x00000001>;
> + ...
> + };
> + ocp {
> + ...
> + phandle = <0x00000002>;
> + ...
> + };
> + __symbols__ {
> + res="/res";
> + ocp="/ocp";
> + };
> +};
> +
> +Notice that all the nodes that had a label have been recorded, and that
> +phandles have been generated for them.
> +
> +This blob can be used to boot the board normally, the __symbols__ node will
> +be safely ignored both by the bootloader and the kernel (the only loss will
> +be a few bytes of memory and disk space).
> +
> +3.b) The Device Tree fragments must be compiled with the same option but they
> +must also have a tag (/plugin/) that allows undefined references to nodes
> +that are not present at compilation time to be recorded so that the runtime
> +loader can fix them.
> +
> +So the bar peripheral's DTS format would be of the form:
> +
> +/dts-v1/ /plugin/; /* allow undefined references and record them */
> +/ {
> + .... /* various properties for loader use; i.e. part id etc. */
> + fragment@0 {
> + target = <&ocp>;
> + __overlay__ {
> + /* bar peripheral */
> + bar {
> + compatible = "corp,bar";
> + ... /* various properties and child nodes */
> + }
> + };
> + };
> +};
The last version of your patches that I tested did not require specifying
the target property, the fragment node, and the __overlay__ node. dtc
properly created all of those items automatically. For example, I could
go to all of the trouble of creating those items in a dts like:
$ cat example_1_hand_coded.dts
/dts-v1/;
/plugin/;
/ {
fragment@0 {
target = <&am3353x_pinmux>;
__overlay__ {
i2c1_pins: pinmux_i2c1_pins {
pinctrl-single,pins = <
0x158 0x72
0x15c 0x72
>;
};
};
};
fragment@1 {
target = <&i2c1>;
__overlay__ {
pinctrl-names = "default";
pinctrl-0 = <&i2c1_pins>;
clock-frequency = <400000>;
status = "okay";
at24@50 {
compatible = "at,24c256";
pagesize = <64>;
reg = <0x50>;
};
};
};
};
Or I could let dtc automagically create all the special features
(target, fragment, __overlay__) from an equivalent dts:
$ cat example_1.dts
/dts-v1/;
/plugin/;
&am3353x_pinmux {
i2c1_pins: pinmux_i2c1_pins {
pinctrl-single,pins = <
0x158 0x72
0x15c 0x72
>;
};
};
&i2c1 {
#address-cells = <1>;
#size-cells = <0>;
pinctrl-names = "default";
pinctrl-0 = <&i2c1_pins>;
clock-frequency = <400000>;
status = "okay";
at24@50 {
compatible = "at,24c256";
pagesize = <64>;
reg = <0x50>;
};
};
I would much prefer that people never hand code the target, fragment, and
__overlay__ in a dts source file. Exposing them at the source level adds
complexity, confusion, and an increased chance of creating an invalid
overlay dtb.
If possible, I would prefer target, fragment, and __overlay__ not be valid
input to dtc. It would probably be difficult to prohibit target and fragment,
because however unlikely they are as property and node names, they are valid
dts syntax before adding the overlay enhancements to dtc. However __overlay__
is not a valid node name without the overlay enhancements and could remain
invalid dts input.
I prefer that target, fragment, and __overlay__ be documented as a dtb to
target system API. In this case, for the normal developer, they are
hidden in the binary dtb format and in the kernel (or boot loader)
overlay framework code.
I do recognize that if __overlay__ is not valid dtc input then it is not
possible to decompile an overlay into a dts containing __overlay__ and
then recompile that dts. This could be resolved by a more complex
decompile that turned the overlay dtb back into the form of example_1.dts
above.
After reading to the end of this patch, I see that the simpler form of
.dts (like example_1.dts) is also noted as "an alternative syntax to
the expanded form for overlays".
> +
> +Note that there's a target property that specifies the location where the
> +contents of the overlay node will be placed, and it references the node
> +in the foo.dts file.
> +
> +$ dtc -@ -O dtb -o bar.dtbo -b 0 bar.dts
> +$ fdtdump bar.dtbo
> +...
> +/ {
> + ... /* properties */
> + fragment@0 {
> + target = <0xffffffff>;
> + __overlay__ {
> + bar {
> + compatible = "corp,bar";
> + ... /* various properties and child nodes */
> + }
> + };
> + };
> + __fixups__ {
> + ocp = "/fragment@0:target:0";
> + };
> +};
> +
> +No __symbols__ has been generated (no label in bar.dts).
> +Note that the target's ocp label is undefined, so the phandle handle
> +value is filled with the illegal value '0xffffffff', while a __fixups__
> +node has been generated, which marks the location in the tree where
> +the label lookup should store the runtime phandle value of the ocp node.
> +
> +The format of the __fixups__ node entry is
> +
> + <label> = "<local-full-path>:<property-name>:<offset>";
> +
> +<label> Is the label we're referring
> +<local-full-path> Is the full path of the node the reference is
> +<property-name> Is the name of the property containing the
> + reference
> +<offset> The offset (in bytes) of where the property's
> + phandle value is located.
> +
> +Doing the same with the baz peripheral's DTS format is a little bit more
> +involved, since baz contains references to local labels which require
> +local fixups.
> +
> +/dts-v1/ /plugin/; /* allow undefined label references and record them */
> +/ {
> + .... /* various properties for loader use; i.e. part id etc. */
> + fragment@0 {
> + target = <&res>;
> + __overlay__ {
> + /* baz resources */
> + baz_res: res_baz { ... };
> + };
> + };
> + fragment@1 {
> + target = <&ocp>;
> + __overlay__ {
> + /* baz peripheral */
> + baz {
> + compatible = "corp,baz";
> + /* reference to another point in the tree */
> + ref-to-res = <&baz_res>;
> + ... /* various properties and child nodes */
> + }
> + };
> + };
> +};
> +
> +Note that &bar_res reference.
> +
> +$ dtc -@ -O dtb -o baz.dtbo -b 0 baz.dts
> +$ fdtdump baz.dtbo
> +...
> +/ {
> + ... /* properties */
> + fragment@0 {
> + target = <0xffffffff>;
> + __overlay__ {
> + res_baz {
> + ....
> + phandle = <0x00000001>;
> + };
> + };
> + };
> + fragment@1 {
> + target = <0xffffffff>;
> + __overlay__ {
> + baz {
> + compatible = "corp,baz";
> + ... /* various properties and child nodes */
> + ref-to-res = <0x00000001>;
> + }
> + };
> + };
> + __fixups__ {
> + res = "/fragment@0:target:0";
> + ocp = "/fragment@1:target:0";
> + };
> + __local_fixups__ {
> + fragment@1 {
> + __overlay__ {
> + baz {
> + ref-to-res = <0>;
> + };
> + };
> + };
> + };
> +};
> +
> +This is similar to the bar case, but the reference of a local label by the
> +baz node generates a __local_fixups__ entry that records the place that the
> +local reference is being made. No matter how phandles are allocated from dtc
> +the run time loader must apply an offset to each phandle in every dynamic
> +DT object loaded. The __local_fixups__ node records the place of every
> +local reference so that the loader can apply the offset.
> +
> +There is an alternative syntax to the expanded form for overlays with phandle
> +targets which makes the format similar to the one using in .dtsi include files.
> +
> +So for the &ocp target example above one can simply write:
> +
> +/dts-v1/ /plugin/;
> +&ocp {
> + /* bar peripheral */
> + bar {
> + compatible = "corp,bar";
> + ... /* various properties and child nodes */
> + }
> +};
> +
> +The resulting dtb object is identical.
>
^ permalink raw reply
* Re: [PATCH v10 2/4] dtc: Document the dynamic plugin internals
From: David Gibson @ 2016-11-29 2:01 UTC (permalink / raw)
To: Stephen Boyd
Cc: Pantelis Antoniou, Jon Loeliger, Grant Likely, Frank Rowand,
Rob Herring, Jan Luebbe, Sascha Hauer, Phil Elwell, Simon Glass,
Maxime Ripard, Thomas Petazzoni, Boris Brezillon, Antoine Tenart,
Devicetree Compiler, devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <148036343076.23275.14028691096221007535@sboyd-linaro>
[-- Attachment #1: Type: text/plain, Size: 15643 bytes --]
On Mon, Nov 28, 2016 at 12:03:50PM -0800, Stephen Boyd wrote:
> Quoting Pantelis Antoniou (2016-11-25 04:32:09)
> > diff --git a/Documentation/dt-object-internal.txt b/Documentation/dt-object-internal.txt
> > new file mode 100644
> > index 0000000..d5b841e
> > --- /dev/null
> > +++ b/Documentation/dt-object-internal.txt
> > @@ -0,0 +1,318 @@
> > +Device Tree Dynamic Object format internals
> > +-------------------------------------------
> > +
> > +The Device Tree for most platforms is a static representation of
> > +the hardware capabilities. This is insufficient for many platforms
>
> s/many//
>
> > +that need to dynamically insert device tree fragments to the
>
> that need to dynamically insert device tree fragments into the
>
> Also, should device tree be capitalized here?
>
> > +running kernel's live tree.
>
> Drop "running kernel's" as it's implicit with "live tree"?
>
> > +
> > +This document explains the the device tree object format and the
>
> s/the//
>
> > +modifications made to the device tree compiler, which make it possible.
> > +
> > +1. Simplified Problem Definition
> > +--------------------------------
> > +
> > +Assume we have a platform which boots using following simplified device tree.
> > +
> > +---- foo.dts -----------------------------------------------------------------
> > + /* FOO platform */
> > + / {
> > + compatible = "corp,foo";
> > +
> > + /* shared resources */
> > + res: res {
> > + };
> > +
> > + /* On chip peripherals */
> > + ocp: ocp {
> > + /* peripherals that are always instantiated */
> > + peripheral1 { ... };
> > + };
> > + };
> > +---- foo.dts -----------------------------------------------------------------
> > +
> > +We have a number of peripherals that after probing (using some undefined method)
> > +should result in different device tree configuration.
> > +
> > +We cannot boot with this static tree because due to the configuration of the
> > +foo platform there exist multiple conficting peripherals DT fragments.
> > +
> > +So for the bar peripheral we would have this:
> > +
> > +---- foo+bar.dts -------------------------------------------------------------
> > + /* FOO platform + bar peripheral */
> > + / {
> > + compatible = "corp,foo";
> > +
> > + /* shared resources */
> > + res: res {
> > + };
> > +
> > + /* On chip peripherals */
> > + ocp: ocp {
> > + /* peripherals that are always instantiated */
> > + peripheral1 { ... };
> > +
> > + /* bar peripheral */
> > + bar {
> > + compatible = "corp,bar";
> > + ... /* various properties and child nodes */
> > + };
> > + };
> > + };
> > +---- foo+bar.dts -------------------------------------------------------------
> > +
> > +While for the baz peripheral we would have this:
> > +
> > +---- foo+baz.dts -------------------------------------------------------------
> > + /* FOO platform + baz peripheral */
> > + / {
> > + compatible = "corp,foo";
> > +
> > + /* shared resources */
> > + res: res {
> > + /* baz resources */
> > + baz_res: res_baz { ... };
> > + };
> > +
> > + /* On chip peripherals */
> > + ocp: ocp {
> > + /* peripherals that are always instantiated */
> > + peripheral1 { ... };
> > +
> > + /* baz peripheral */
> > + baz {
> > + compatible = "corp,baz";
> > + /* reference to another point in the tree */
> > + ref-to-res = <&baz_res>;
> > + ... /* various properties and child nodes */
> > + };
> > + };
> > + };
> > +---- foo+baz.dts -------------------------------------------------------------
> > +
> > +We note that the baz case is more complicated, since the baz peripheral needs to
> > +reference another node in the DT tree.
> > +
> > +2. Device Tree Object Format Requirements
> > +-----------------------------------------
> > +
> > +Since the device tree is used for booting a number of very different hardware
> > +platforms it is imperative that we tread very carefully.
> > +
> > +2.a) No changes to the Device Tree binary format for the base tree. We cannot
> > +modify the tree format at all and all the information we require should be
> > +encoded using device tree itself. We can add nodes that can be safely ignored
> > +by both bootloaders and the kernel. The plugin dtb's are optionally tagged
>
> s/dtb's/dtbs/
>
> > +with a different magic number in the header but otherwise they too are simple
> > +blobs.
>
> but otherwise they're simple blobs.
>
> > +
> > +2.b) Changes to the DTS source format should be absolutely minimal, and should
> > +only be needed for the DT fragment definitions, and not the base boot DT.
> > +
> > +2.c) An explicit option should be used to instruct DTC to generate the required
> > +information needed for object resolution. Platforms that don't use the
> > +dynamic object format can safely ignore it.
>
> Why? We can't figure that out based on the /plugin/ label within the dts
> file? And shouldn't we always generate a __symbols__ node in the base
> dtb?
No, given it's a nonstandard extension on the basic device tree
contents, I don't think we should generate the symbol information by
default. /plugin/ can let you determine whether to generate fixups,
but you need the symbols for the base tree.
> > +
> > +2.d) Finally, DT syntax changes should be kept to a minimum. It should be
> > +possible to express everything using the existing DT syntax.
> > +
> > +3. Implementation
> > +-----------------
> > +
> > +The basic unit of addressing in Device Tree is the phandle. Turns out it's
> > +relatively simple to extend the way phandles are generated and referenced
> > +so that it's possible to dynamically convert symbolic references (labels)
> > +to phandle values. This is a valid assumption as long as the author uses
> > +reference syntax and does not assign phandle values manually (which might
> > +be a problem with decompiled source files).
> > +
> > +We can roughly divide the operation into two steps.
> > +
> > +3.a) Compilation of the base board DTS file using the '-@' option
> > +generates a valid DT blob with an added __symbols__ node at the root node,
> > +containing a list of all nodes that are marked with a label.
> > +
> > +Using the foo.dts file above the following node will be generated;
> > +
> > +$ dtc -@ -O dtb -o foo.dtb -b 0 foo.dts
> > +$ fdtdump foo.dtb
> > +...
> > +/ {
> > + ...
> > + res {
> > + ...
> > + phandle = <0x00000001>;
> > + ...
> > + };
> > + ocp {
> > + ...
> > + phandle = <0x00000002>;
> > + ...
> > + };
> > + __symbols__ {
> > + res="/res";
> > + ocp="/ocp";
> > + };
> > +};
> > +
> > +Notice that all the nodes that had a label have been recorded, and that
> > +phandles have been generated for them.
> > +
> > +This blob can be used to boot the board normally, the __symbols__ node will
> > +be safely ignored both by the bootloader and the kernel (the only loss will
> > +be a few bytes of memory and disk space).
>
> This never really mentions why we need to generate a symbols node.
> Perhaps we should say something like "we generate a __symbols__ node to
> record nodes that had labels in the base tree so they can be matched up
> with the fragments which reference the same labels"? Or something like
> that.
>
> I also wonder why it's even necessary. Couldn't we require overlays to
> be compiled with the original dts files? Then we could encode the full
> path of nodes referenced in the overlay into the overlay dtb itself.
That's one of many different design decisions that could have been
made, and might have been a better idea. But the current design is
out in the wild now, flaws and all, so we do need to implement it.
> > +
> > +3.b) The Device Tree fragments must be compiled with the same option but they
> > +must also have a tag (/plugin/) that allows undefined references to nodes
> > +that are not present at compilation time to be recorded so that the runtime
> > +loader can fix them.
> > +
> > +So the bar peripheral's DTS format would be of the form:
> > +
> > +/dts-v1/ /plugin/; /* allow undefined references and record them */
> > +/ {
> > + .... /* various properties for loader use; i.e. part id etc. */
> > + fragment@0 {
> > + target = <&ocp>;
> > + __overlay__ {
> > + /* bar peripheral */
> > + bar {
> > + compatible = "corp,bar";
> > + ... /* various properties and child nodes */
> > + }
> > + };
> > + };
> > +};
> > +
> > +Note that there's a target property that specifies the location where the
> > +contents of the overlay node will be placed, and it references the node
> > +in the foo.dts file.
> > +
> > +$ dtc -@ -O dtb -o bar.dtbo -b 0 bar.dts
> > +$ fdtdump bar.dtbo
> > +...
> > +/ {
> > + ... /* properties */
> > + fragment@0 {
> > + target = <0xffffffff>;
> > + __overlay__ {
> > + bar {
> > + compatible = "corp,bar";
> > + ... /* various properties and child nodes */
> > + }
> > + };
> > + };
> > + __fixups__ {
> > + ocp = "/fragment@0:target:0";
> > + };
> > +};
> > +
> > +No __symbols__ has been generated (no label in bar.dts).
>
> Add "node" after __symbols__ here?
>
> > +Note that the target's ocp label is undefined, so the phandle handle
>
> Drop handle after phandle?
>
> > +value is filled with the illegal value '0xffffffff', while a __fixups__
> > +node has been generated, which marks the location in the tree where
> > +the label lookup should store the runtime phandle value of the ocp node.
> > +
> > +The format of the __fixups__ node entry is
> > +
> > + <label> = "<local-full-path>:<property-name>:<offset>";
> > +
> > +<label> Is the label we're referring
> > +<local-full-path> Is the full path of the node the reference is
> > +<property-name> Is the name of the property containing the
>
> Weird alignment here.
>
> > + reference
> > +<offset> The offset (in bytes) of where the property's
> > + phandle value is located.
>
> located within the property? Or "offset relative to the start of the
> property in bytes where the phandle value is located"?
>
> Is this a list? So multiple properties can be fixed up with the same
> label? If so that isn't clear from this description.
>
> > +
> > +Doing the same with the baz peripheral's DTS format is a little bit more
> > +involved, since baz contains references to local labels which require
> > +local fixups.
> > +
> > +/dts-v1/ /plugin/; /* allow undefined label references and record them */
> > +/ {
> > + .... /* various properties for loader use; i.e. part id etc. */
> > + fragment@0 {
> > + target = <&res>;
> > + __overlay__ {
> > + /* baz resources */
> > + baz_res: res_baz { ... };
> > + };
> > + };
> > + fragment@1 {
> > + target = <&ocp>;
> > + __overlay__ {
> > + /* baz peripheral */
> > + baz {
> > + compatible = "corp,baz";
> > + /* reference to another point in the tree */
> > + ref-to-res = <&baz_res>;
> > + ... /* various properties and child nodes */
> > + }
> > + };
> > + };
> > +};
> > +
> > +Note that &bar_res reference.
> > +
> > +$ dtc -@ -O dtb -o baz.dtbo -b 0 baz.dts
> > +$ fdtdump baz.dtbo
> > +...
> > +/ {
> > + ... /* properties */
> > + fragment@0 {
> > + target = <0xffffffff>;
> > + __overlay__ {
> > + res_baz {
> > + ....
> > + phandle = <0x00000001>;
> > + };
> > + };
> > + };
> > + fragment@1 {
> > + target = <0xffffffff>;
> > + __overlay__ {
> > + baz {
> > + compatible = "corp,baz";
> > + ... /* various properties and child nodes */
> > + ref-to-res = <0x00000001>;
> > + }
> > + };
> > + };
> > + __fixups__ {
> > + res = "/fragment@0:target:0";
> > + ocp = "/fragment@1:target:0";
> > + };
> > + __local_fixups__ {
> > + fragment@1 {
> > + __overlay__ {
> > + baz {
> > + ref-to-res = <0>;
> > + };
> > + };
> > + };
> > + };
> > +};
> > +
> > +This is similar to the bar case, but the reference of a local label by the
> > +baz node generates a __local_fixups__ entry that records the place that the
> > +local reference is being made. No matter how phandles are allocated from dtc
> > +the run time loader must apply an offset to each phandle in every dynamic
> > +DT object loaded. The __local_fixups__ node records the place of every
>
> records the offset relative to the start of the property of every local
> reference within that property so that the loader...
>
> > +local reference so that the loader can apply the offset.
> > +
> > +There is an alternative syntax to the expanded form for overlays with phandle
> > +targets which makes the format similar to the one using in .dtsi include files.
> > +
> > +So for the &ocp target example above one can simply write:
> > +
> > +/dts-v1/ /plugin/;
> > +&ocp {
> > + /* bar peripheral */
> > + bar {
> > + compatible = "corp,bar";
> > + ... /* various properties and child nodes */
> > + }
> > +};
> > +
> > +The resulting dtb object is identical.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v10 2/4] dtc: Document the dynamic plugin internals
From: David Gibson @ 2016-11-29 2:04 UTC (permalink / raw)
To: Pantelis Antoniou
Cc: Stephen Boyd, Jon Loeliger, Grant Likely, Frank Rowand,
Rob Herring, Jan Luebbe, Sascha Hauer, Phil Elwell, Simon Glass,
Maxime Ripard, Thomas Petazzoni, Boris Brezillon, Antoine Tenart,
Devicetree Compiler, devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <D1B6ABA4-34A3-42BA-9B10-85CAE4DA6A28-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 9943 bytes --]
On Mon, Nov 28, 2016 at 10:29:05PM +0200, Pantelis Antoniou wrote:
> Hi Stephen,
>
> > On Nov 28, 2016, at 22:03 , Stephen Boyd <stephen.boyd-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> >
> > Quoting Pantelis Antoniou (2016-11-25 04:32:09)
> >> diff --git a/Documentation/dt-object-internal.txt b/Documentation/dt-object-internal.txt
> >> new file mode 100644
> >> index 0000000..d5b841e
> >> --- /dev/null
> >> +++ b/Documentation/dt-object-internal.txt
> >> @@ -0,0 +1,318 @@
> >> +Device Tree Dynamic Object format internals
> >> +-------------------------------------------
> >> +
> >> +The Device Tree for most platforms is a static representation of
> >> +the hardware capabilities. This is insufficient for many platforms
> >
> > s/many//
> >
> >> +that need to dynamically insert device tree fragments to the
> >
> > that need to dynamically insert device tree fragments into the
> >
> > Also, should device tree be capitalized here?
> >
> >> +running kernel's live tree.
> >
> > Drop "running kernel's" as it's implicit with "live tree"?
> >
> >> +
> >> +This document explains the the device tree object format and the
> >
> > s/the//
> >
> >> +modifications made to the device tree compiler, which make it possible.
> >> +
> >> +1. Simplified Problem Definition
> >> +--------------------------------
> >> +
> >> +Assume we have a platform which boots using following simplified device tree.
> >> +
> >> +---- foo.dts -----------------------------------------------------------------
> >> + /* FOO platform */
> >> + / {
> >> + compatible = "corp,foo";
> >> +
> >> + /* shared resources */
> >> + res: res {
> >> + };
> >> +
> >> + /* On chip peripherals */
> >> + ocp: ocp {
> >> + /* peripherals that are always instantiated */
> >> + peripheral1 { ... };
> >> + };
> >> + };
> >> +---- foo.dts -----------------------------------------------------------------
> >> +
> >> +We have a number of peripherals that after probing (using some undefined method)
> >> +should result in different device tree configuration.
> >> +
> >> +We cannot boot with this static tree because due to the configuration of the
> >> +foo platform there exist multiple conficting peripherals DT fragments.
> >> +
> >> +So for the bar peripheral we would have this:
> >> +
> >> +---- foo+bar.dts -------------------------------------------------------------
> >> + /* FOO platform + bar peripheral */
> >> + / {
> >> + compatible = "corp,foo";
> >> +
> >> + /* shared resources */
> >> + res: res {
> >> + };
> >> +
> >> + /* On chip peripherals */
> >> + ocp: ocp {
> >> + /* peripherals that are always instantiated */
> >> + peripheral1 { ... };
> >> +
> >> + /* bar peripheral */
> >> + bar {
> >> + compatible = "corp,bar";
> >> + ... /* various properties and child nodes */
> >> + };
> >> + };
> >> + };
> >> +---- foo+bar.dts -------------------------------------------------------------
> >> +
> >> +While for the baz peripheral we would have this:
> >> +
> >> +---- foo+baz.dts -------------------------------------------------------------
> >> + /* FOO platform + baz peripheral */
> >> + / {
> >> + compatible = "corp,foo";
> >> +
> >> + /* shared resources */
> >> + res: res {
> >> + /* baz resources */
> >> + baz_res: res_baz { ... };
> >> + };
> >> +
> >> + /* On chip peripherals */
> >> + ocp: ocp {
> >> + /* peripherals that are always instantiated */
> >> + peripheral1 { ... };
> >> +
> >> + /* baz peripheral */
> >> + baz {
> >> + compatible = "corp,baz";
> >> + /* reference to another point in the tree */
> >> + ref-to-res = <&baz_res>;
> >> + ... /* various properties and child nodes */
> >> + };
> >> + };
> >> + };
> >> +---- foo+baz.dts -------------------------------------------------------------
> >> +
> >> +We note that the baz case is more complicated, since the baz peripheral needs to
> >> +reference another node in the DT tree.
> >> +
> >> +2. Device Tree Object Format Requirements
> >> +-----------------------------------------
> >> +
> >> +Since the device tree is used for booting a number of very different hardware
> >> +platforms it is imperative that we tread very carefully.
> >> +
> >> +2.a) No changes to the Device Tree binary format for the base tree. We cannot
> >> +modify the tree format at all and all the information we require should be
> >> +encoded using device tree itself. We can add nodes that can be safely ignored
> >> +by both bootloaders and the kernel. The plugin dtb's are optionally tagged
> >
> > s/dtb's/dtbs/
> >
> >> +with a different magic number in the header but otherwise they too are simple
> >> +blobs.
> >
> > but otherwise they're simple blobs.
> >
>
> OK on the spelling/grammar fixes above.
>
> >> +
> >> +2.b) Changes to the DTS source format should be absolutely minimal, and should
> >> +only be needed for the DT fragment definitions, and not the base boot DT.
> >> +
> >> +2.c) An explicit option should be used to instruct DTC to generate the required
> >> +information needed for object resolution. Platforms that don't use the
> >> +dynamic object format can safely ignore it.
> >
> > Why? We can't figure that out based on the /plugin/ label within the dts
> > file? And shouldn't we always generate a __symbols__ node in the base
> > dtb?
> >
>
> Actually now we do. The last patchset does automatically generate those nodes
> if a /plugin/ tag is encountered. For base dtbs I would suggest that generating
> the symbols node automatically is what’s sane too, but unfortunately there are
> some platforms out there that are having trouble with larger dtbs than what they
> expect.
>
> It is your call whether to enable it by default I guess.
>
> >> +
> >> +2.d) Finally, DT syntax changes should be kept to a minimum. It should be
> >> +possible to express everything using the existing DT syntax.
> >> +
> >> +3. Implementation
> >> +-----------------
> >> +
> >> +The basic unit of addressing in Device Tree is the phandle. Turns out it's
> >> +relatively simple to extend the way phandles are generated and referenced
> >> +so that it's possible to dynamically convert symbolic references (labels)
> >> +to phandle values. This is a valid assumption as long as the author uses
> >> +reference syntax and does not assign phandle values manually (which might
> >> +be a problem with decompiled source files).
> >> +
> >> +We can roughly divide the operation into two steps.
> >> +
> >> +3.a) Compilation of the base board DTS file using the '-@' option
> >> +generates a valid DT blob with an added __symbols__ node at the root node,
> >> +containing a list of all nodes that are marked with a label.
> >> +
> >> +Using the foo.dts file above the following node will be generated;
> >> +
> >> +$ dtc -@ -O dtb -o foo.dtb -b 0 foo.dts
> >> +$ fdtdump foo.dtb
> >> +...
> >> +/ {
> >> + ...
> >> + res {
> >> + ...
> >> + phandle = <0x00000001>;
> >> + ...
> >> + };
> >> + ocp {
> >> + ...
> >> + phandle = <0x00000002>;
> >> + ...
> >> + };
> >> + __symbols__ {
> >> + res="/res";
> >> + ocp="/ocp";
> >> + };
> >> +};
> >> +
> >> +Notice that all the nodes that had a label have been recorded, and that
> >> +phandles have been generated for them.
> >> +
> >> +This blob can be used to boot the board normally, the __symbols__ node will
> >> +be safely ignored both by the bootloader and the kernel (the only loss will
> >> +be a few bytes of memory and disk space).
> >
> > This never really mentions why we need to generate a symbols node.
> > Perhaps we should say something like "we generate a __symbols__ node to
> > record nodes that had labels in the base tree so they can be matched up
> > with the fragments which reference the same labels"? Or something like
> > that.
> >
>
> Hmm, yeah.
>
> > I also wonder why it's even necessary. Couldn't we require overlays to
> > be compiled with the original dts files? Then we could encode the full
> > path of nodes referenced in the overlay into the overlay dtb itself.
> >
>
> No, we can’t do that; the end-game of this is for overlays to be portable
> for use in platforms having the same kind of connectors.
That's kind of true, but actually I think we want to redesign the
connector format. Obviously we'll take some stuff from the current
overlay format, but we can't be fully compatible with them, so we
should take the opportunity to remove some of the sillier design flaws
in the overlay format.
That said, even with their flaws and limitations, overlays in the
current format can sometimes be portable to multiple base trees.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v10 3/4] dtc: Plugin and fixup support
From: David Gibson @ 2016-11-29 2:10 UTC (permalink / raw)
To: Pantelis Antoniou
Cc: Jon Loeliger, Grant Likely, Frank Rowand, Rob Herring, Jan Luebbe,
Sascha Hauer, Phil Elwell, Simon Glass, Maxime Ripard,
Thomas Petazzoni, Boris Brezillon, Antoine Tenart, Stephen Boyd,
Devicetree Compiler, devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <D69908BD-B243-4AEE-B6BA-80B94AFE4B6A-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 13831 bytes --]
On Mon, Nov 28, 2016 at 02:10:35PM +0200, Pantelis Antoniou wrote:
>
> > On Nov 28, 2016, at 06:12 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >
> > On Fri, Nov 25, 2016 at 02:32:10PM +0200, Pantelis Antoniou wrote:
> >> This patch enable the generation of symbols & local fixup information
> >> for trees compiled with the -@ (--symbols) option.
> >>
> >> Using this patch labels in the tree and their users emit information
> >> in __symbols__ and __local_fixups__ nodes.
> >>
> >> The __fixups__ node make possible the dynamic resolution of phandle
> >> references which are present in the plugin tree but lie in the
> >> tree that are applying the overlay against.
> >>
> >> While there is a new magic number for dynamic device tree/overlays blobs
> >> it is by default enabled. Remember to use -M to generate compatible
> >> blobs.
> >>
> >> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> >> Signed-off-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> >> Signed-off-by: Jan Luebbe <jlu-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> >> ---
> >> Documentation/manual.txt | 25 +++++-
> >> checks.c | 8 +-
> >> dtc-lexer.l | 5 ++
> >> dtc-parser.y | 50 +++++++++--
> >> dtc.c | 39 +++++++-
> >> dtc.h | 20 ++++-
> >> fdtdump.c | 2 +-
> >> flattree.c | 17 ++--
> >> fstree.c | 2 +-
> >> libfdt/fdt.c | 2 +-
> >> libfdt/fdt.h | 3 +-
> >> livetree.c | 225 ++++++++++++++++++++++++++++++++++++++++++++++-
> >> tests/mangle-layout.c | 7 +-
> >> 13 files changed, 375 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/Documentation/manual.txt b/Documentation/manual.txt
> >> index 398de32..094893b 100644
> >> --- a/Documentation/manual.txt
> >> +++ b/Documentation/manual.txt
> >> @@ -119,6 +119,24 @@ Options:
> >> Make space for <number> reserve map entries
> >> Relevant for dtb and asm output only.
> >>
> >> + -@
> >> + Generates a __symbols__ node at the root node of the resulting blob
> >> + for any node labels used, and for any local references using phandles
> >> + it also generates a __local_fixups__ node that tracks them.
> >> +
> >> + When using the /plugin/ tag all unresolved label references to
> >> + be tracked in the __fixups__ node, making dynamic resolution possible.
> >> +
> >> + -A
> >> + Generate automatically aliases for all node labels. This is similar to
> >> + the -@ option (the __symbols__ node contain identical information) but
> >> + the semantics are slightly different since no phandles are automatically
> >> + generated for labeled nodes.
> >> +
> >> + -M
> >> + Generate blobs with the old FDT magic number for device tree objects.
> >> + By default blobs use the DTBO FDT magic number instead.
> >> +
> >> -S <bytes>
> >> Ensure the blob at least <bytes> long, adding additional
> >> space if needed.
> >> @@ -146,13 +164,18 @@ Additionally, dtc performs various sanity checks on the tree.
> >> Here is a very rough overview of the layout of a DTS source file:
> >>
> >>
> >> - sourcefile: list_of_memreserve devicetree
> >> + sourcefile: versioninfo plugindecl list_of_memreserve devicetree
> >>
> >> memreserve: label 'memreserve' ADDR ADDR ';'
> >> | label 'memreserve' ADDR '-' ADDR ';'
> >>
> >> devicetree: '/' nodedef
> >>
> >> + versioninfo: '/' 'dts-v1' '/' ';'
> >> +
> >> + plugindecl: '/' 'plugin' '/' ';'
> >> + | /* empty */
> >> +
> >> nodedef: '{' list_of_property list_of_subnode '}' ';'
> >>
> >> property: label PROPNAME '=' propdata ';'
> >> diff --git a/checks.c b/checks.c
> >> index 2bd27a4..4292f4b 100644
> >> --- a/checks.c
> >> +++ b/checks.c
> >> @@ -487,8 +487,12 @@ static void fixup_phandle_references(struct check *c, struct boot_info *bi,
> >>
> >> refnode = get_node_by_ref(dt, m->ref);
> >> if (! refnode) {
> >> - FAIL(c, "Reference to non-existent node or label \"%s\"\n",
> >> - m->ref);
> >> + if (!(bi->versionflags & VF_PLUGIN))
> >> + FAIL(c, "Reference to non-existent node or "
> >> + "label \"%s\"\n", m->ref);
> >> + else /* mark the entry as unresolved */
> >> + *((cell_t *)(prop->val.val + m->offset)) =
> >> + cpu_to_fdt32(0xffffffff);
> >> continue;
> >> }
> >>
> >> diff --git a/dtc-lexer.l b/dtc-lexer.l
> >> index 790fbf6..40bbc87 100644
> >> --- a/dtc-lexer.l
> >> +++ b/dtc-lexer.l
> >> @@ -121,6 +121,11 @@ static void lexical_error(const char *fmt, ...);
> >> return DT_V1;
> >> }
> >>
> >> +<*>"/plugin/" {
> >> + DPRINT("Keyword: /plugin/\n");
> >> + return DT_PLUGIN;
> >> + }
> >> +
> >> <*>"/memreserve/" {
> >> DPRINT("Keyword: /memreserve/\n");
> >> BEGIN_DEFAULT();
> >> diff --git a/dtc-parser.y b/dtc-parser.y
> >> index 14aaf2e..1a1f660 100644
> >> --- a/dtc-parser.y
> >> +++ b/dtc-parser.y
> >> @@ -19,6 +19,7 @@
> >> */
> >> %{
> >> #include <stdio.h>
> >> +#include <inttypes.h>
> >>
> >> #include "dtc.h"
> >> #include "srcpos.h"
> >> @@ -33,6 +34,7 @@ extern void yyerror(char const *s);
> >>
> >> extern struct boot_info *the_boot_info;
> >> extern bool treesource_error;
> >> +
> >
> > Extraneous whitespace change here
> >
>
> OK.
>
> >> %}
> >>
> >> %union {
> >> @@ -52,9 +54,11 @@ extern bool treesource_error;
> >> struct node *nodelist;
> >> struct reserve_info *re;
> >> uint64_t integer;
> >> + unsigned int flags;
> >> }
> >>
> >> %token DT_V1
> >> +%token DT_PLUGIN
> >> %token DT_MEMRESERVE
> >> %token DT_LSHIFT DT_RSHIFT DT_LE DT_GE DT_EQ DT_NE DT_AND DT_OR
> >> %token DT_BITS
> >> @@ -71,6 +75,8 @@ extern bool treesource_error;
> >>
> >> %type <data> propdata
> >> %type <data> propdataprefix
> >> +%type <flags> versioninfo
> >> +%type <flags> plugindecl
> >> %type <re> memreserve
> >> %type <re> memreserves
> >> %type <array> arrayprefix
> >> @@ -101,16 +107,34 @@ extern bool treesource_error;
> >> %%
> >>
> >> sourcefile:
> >> - v1tag memreserves devicetree
> >> + versioninfo plugindecl memreserves devicetree
> >> + {
> >> + the_boot_info = build_boot_info($1 | $2, $3, $4,
> >> + guess_boot_cpuid($4));
> >> + }
> >> + ;
> >> +
> >> +versioninfo:
> >> + v1tag
> >> {
> >> - the_boot_info = build_boot_info($2, $3,
> >> - guess_boot_cpuid($3));
> >> + $$ = VF_DT_V1;
> >> }
> >> ;
> >>
> >> v1tag:
> >> DT_V1 ';'
> >> + | DT_V1
> >> | DT_V1 ';' v1tag
> >> +
> >> +plugindecl:
> >> + DT_PLUGIN ';'
> >> + {
> >> + $$ = VF_PLUGIN;
> >> + }
> >> + | /* empty */
> >> + {
> >> + $$ = 0;
> >> + }
> >> ;
> >>
> >> memreserves:
> >> @@ -161,10 +185,19 @@ devicetree:
> >> {
> >> struct node *target = get_node_by_ref($1, $2);
> >>
> >> - if (target)
> >> + if (target) {
> >> merge_nodes(target, $3);
> >> - else
> >> - ERROR(&@2, "Label or path %s not found", $2);
> >> + } else {
> >> + /*
> >> + * We rely on the rule being always:
> >> + * versioninfo plugindecl memreserves devicetree
> >> + * so $-1 is what we want (plugindecl)
> >> + */
> >> + if ($<flags>-1 & VF_PLUGIN)
> >
> > o_O... ok. I've never seen negative value references before. Can you
> > provide a link to some documentation saying this is actually supported
> > usage in bison? I wasn't able to find it when I looked.
> >
>
> There is a section about inherited attributes in the flex & bison book by O’Reily.
>
> https://books.google.gr/books?id=3Sr1V5J9_qMC&lpg=PP1&dq=flex%20bison&hl=el&pg=PP1#v=onepage&q=flex%20bison&f=false
>
> There’s a direct link to the 2nd Edition of lex & yacc:
>
> https://books.google.gr/books?id=fMPxfWfe67EC&lpg=PA183&ots=RcRSji2NAT&dq=yacc%20inherited%20attributes&hl=el&pg=PA183#v=onepage&q=yacc%20inherited%20attributes&f=false
Thanks for the link. I still think moving the fragment assembly out
of the parser will be a better idea long term, but this does address
the main concern I had, so it will do for now.
> >> + add_orphan_node($1, $3, $2);
> >> + else
> >> + ERROR(&@2, "Label or path %s not found", $2);
> >> + }
> >> $$ = $1;
> >> }
> >> | devicetree DT_DEL_NODE DT_REF ';'
> >> @@ -179,6 +212,11 @@ devicetree:
> >>
> >> $$ = $1;
> >> }
> >> + | /* empty */
> >> + {
> >> + /* build empty node */
> >> + $$ = name_node(build_node(NULL, NULL), "");
> >> + }
> >> ;
> >>
> >> nodedef:
> >> diff --git a/dtc.c b/dtc.c
> >> index 9dcf640..06e91bc 100644
> >> --- a/dtc.c
> >> +++ b/dtc.c
> >> @@ -32,6 +32,9 @@ int minsize; /* Minimum blob size */
> >> int padsize; /* Additional padding to blob */
> >> int alignsize; /* Additional padding to blob accroding to the alignsize */
> >> int phandle_format = PHANDLE_BOTH; /* Use linux,phandle or phandle properties */
> >> +int symbol_fixup_support; /* enable symbols & fixup support */
> >> +int auto_label_aliases; /* auto generate labels -> aliases */
> >> +int no_dtbo_magic; /* use old FDT magic values for objects */
> >>
> >> static int is_power_of_2(int x)
> >> {
> >> @@ -59,7 +62,7 @@ static void fill_fullpaths(struct node *tree, const char *prefix)
> >> #define FDT_VERSION(version) _FDT_VERSION(version)
> >> #define _FDT_VERSION(version) #version
> >> static const char usage_synopsis[] = "dtc [options] <input file>";
> >> -static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:a:fb:i:H:sW:E:hv";
> >> +static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:a:fb:i:H:sW:E:@AMhv";
> >> static struct option const usage_long_opts[] = {
> >> {"quiet", no_argument, NULL, 'q'},
> >> {"in-format", a_argument, NULL, 'I'},
> >> @@ -78,6 +81,9 @@ static struct option const usage_long_opts[] = {
> >> {"phandle", a_argument, NULL, 'H'},
> >> {"warning", a_argument, NULL, 'W'},
> >> {"error", a_argument, NULL, 'E'},
> >> + {"symbols", no_argument, NULL, '@'},
> >> + {"auto-alias", no_argument, NULL, 'A'},
> >> + {"no-dtbo-magic", no_argument, NULL, 'M'},
> >> {"help", no_argument, NULL, 'h'},
> >> {"version", no_argument, NULL, 'v'},
> >> {NULL, no_argument, NULL, 0x0},
> >> @@ -109,6 +115,9 @@ static const char * const usage_opts_help[] = {
> >> "\t\tboth - Both \"linux,phandle\" and \"phandle\" properties",
> >> "\n\tEnable/disable warnings (prefix with \"no-\")",
> >> "\n\tEnable/disable errors (prefix with \"no-\")",
> >> + "\n\tEnable symbols/fixup support",
> >> + "\n\tEnable auto-alias of labels",
> >> + "\n\tDo not use DTBO magic value for plugin objects",
> >> "\n\tPrint this help and exit",
> >> "\n\tPrint version and exit",
> >> NULL,
> >> @@ -153,7 +162,7 @@ static const char *guess_input_format(const char *fname, const char *fallback)
> >> fclose(f);
> >>
> >> magic = fdt32_to_cpu(magic);
> >> - if (magic == FDT_MAGIC)
> >> + if (magic == FDT_MAGIC || magic == FDT_MAGIC_DTBO)
> >> return "dtb";
> >>
> >> return guess_type_by_name(fname, fallback);
> >> @@ -172,6 +181,7 @@ int main(int argc, char *argv[])
> >> FILE *outf = NULL;
> >> int outversion = DEFAULT_FDT_VERSION;
> >> long long cmdline_boot_cpuid = -1;
> >> + fdt32_t out_magic = FDT_MAGIC;
> >>
> >> quiet = 0;
> >> reservenum = 0;
> >> @@ -249,6 +259,16 @@ int main(int argc, char *argv[])
> >> parse_checks_option(false, true, optarg);
> >> break;
> >>
> >> + case '@':
> >> + symbol_fixup_support = 1;
> >> + break;
> >> + case 'A':
> >> + auto_label_aliases = 1;
> >> + break;
> >> + case 'M':
> >> + no_dtbo_magic = 1;
> >> + break;
> >> +
> >> case 'h':
> >> usage(NULL);
> >> default:
> >> @@ -306,6 +326,14 @@ int main(int argc, char *argv[])
> >> fill_fullpaths(bi->dt, "");
> >> process_checks(force, bi);
> >>
> >> + if (auto_label_aliases)
> >> + generate_label_tree(bi->dt, "aliases", false);
> >> +
> >> + if (symbol_fixup_support) {
> >> + generate_label_tree(bi->dt, "__symbols__", true);
> >> + generate_fixups_tree(bi->dt);
> >
> > Hang on.. this doesn't seem right. I thought -@ controlled the
> > __symbols__ side (i.e. the part upon which we overlay) rather than the
> > fixups side (the part which overlays). A dtbo could certainly have
> > both, of course, but for base trees, wouldn't you have symbols without
> > fixups? And should it be illegal to try to build a /plugin/ without
> > -@?
>
> It does control both for now. For base trees having the fixup nodes
> will allow us to do probe order dependency tracking in the future.
Erm.. how?
> For plugins we need the __symbols__ node to support stacked overlays, i.e.
> overlays referring label that were introduced by a previous overlay.
Yes, I realise that an overlay may well want __symbols__ as well. But
they still seem conceptually different. I think -@ should control
__symbols__ whereas /plugin/ should control __fixups__.
> For plugins there is no requirement for now to actually contain references to
> be resolved. It can easily be enforced though.
Sure, but I don't see the relevance of that here. You could just omit
the __fixups__ node if there's nothing to go into them.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v10 3/4] dtc: Plugin and fixup support
From: David Gibson @ 2016-11-29 2:11 UTC (permalink / raw)
To: Phil Elwell
Cc: Pantelis Antoniou, Jon Loeliger, Grant Likely, Frank Rowand,
Rob Herring, Jan Luebbe, Sascha Hauer, Simon Glass, Maxime Ripard,
Thomas Petazzoni, Boris Brezillon, Antoine Tenart, Stephen Boyd,
Devicetree Compiler, devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <4672e164-aae0-6306-fe70-146a1f930cf7-FnsA7b+Nu9XbIbC87yuRow@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1065 bytes --]
On Mon, Nov 28, 2016 at 12:24:20PM +0000, Phil Elwell wrote:
> On 28/11/2016 12:10, Pantelis Antoniou wrote:
> > For plugins we need the __symbols__ node to support stacked overlays, i.e.
> > overlays referring label that were introduced by a previous overlay.
> Although it is arguably useful to be able to refer to symbols created by
> one overlay from within another, do we really want all symbols to be
> global? Isn't there a call for a new syntax or usage pattern to indicate
> either that a symbol should be local to the overlay or, my preferred
> option, global?
So, this is back to a design question about the overlay format. As
noted in the initial discussions about possible "connector" formats, I
think we will want some sort of local symbols. But the current
overlay format with all global symbols is out there and we need to
support it.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* [PATCH] EDAC: mpc85xx: Add T2080 l2-cache support
From: Chris Packham @ 2016-11-29 2:20 UTC (permalink / raw)
To: linux-edac
Cc: Chris Packham, Rob Herring, Mark Rutland, Benjamin Herrenschmidt,
Paul Mackerras, Michael Ellerman, Johannes Thumshirn,
Borislav Petkov, Mauro Carvalho Chehab, devicetree, linuxppc-dev,
linux-kernel
The l2-cache controller on the T2080 SoC has similar capabilities to the
others already supported by the mpc85xx_edac driver. Add it to the list
of compatible devices.
Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
arch/powerpc/boot/dts/fsl/t2081si-post.dtsi | 1 +
drivers/edac/mpc85xx_edac.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/arch/powerpc/boot/dts/fsl/t2081si-post.dtsi b/arch/powerpc/boot/dts/fsl/t2081si-post.dtsi
index c744569a20e1..a97296c64eb2 100644
--- a/arch/powerpc/boot/dts/fsl/t2081si-post.dtsi
+++ b/arch/powerpc/boot/dts/fsl/t2081si-post.dtsi
@@ -678,5 +678,6 @@
compatible = "fsl,t2080-l2-cache-controller";
reg = <0xc20000 0x40000>;
next-level-cache = <&cpc>;
+ interrupts = <16 2 1 9>;
};
};
diff --git a/drivers/edac/mpc85xx_edac.c b/drivers/edac/mpc85xx_edac.c
index ff0567526ee3..aee6dcdae02a 100644
--- a/drivers/edac/mpc85xx_edac.c
+++ b/drivers/edac/mpc85xx_edac.c
@@ -613,6 +613,7 @@ static const struct of_device_id mpc85xx_l2_err_of_match[] = {
{ .compatible = "fsl,p1020-l2-cache-controller", },
{ .compatible = "fsl,p1021-l2-cache-controller", },
{ .compatible = "fsl,p2020-l2-cache-controller", },
+ { .compatible = "fsl,t2080-l2-cache-controller", },
{},
};
MODULE_DEVICE_TABLE(of, mpc85xx_l2_err_of_match);
--
2.10.2
^ permalink raw reply related
* Re: [PATCH v11 2/7] dtc: Plugin and fixup support
From: David Gibson @ 2016-11-29 2:38 UTC (permalink / raw)
To: Pantelis Antoniou
Cc: Jon Loeliger, Grant Likely, Frank Rowand, Rob Herring, Jan Luebbe,
Sascha Hauer, Phil Elwell, Simon Glass, Maxime Ripard,
Thomas Petazzoni, Boris Brezillon, Antoine Tenart, Stephen Boyd,
Devicetree Compiler, devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1480349141-14145-3-git-send-email-pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 27551 bytes --]
On Mon, Nov 28, 2016 at 06:05:36PM +0200, Pantelis Antoniou wrote:
> This patch enable the generation of symbols & local fixup information
> for trees compiled with the -@ (--symbols) option.
>
> Using this patch labels in the tree and their users emit information
> in __symbols__ and __local_fixups__ nodes.
>
> The __fixups__ node make possible the dynamic resolution of phandle
> references which are present in the plugin tree but lie in the
> tree that are applying the overlay against.
>
> While there is a new magic number for dynamic device tree/overlays blobs
> it is by default enabled. Remember to use -M to generate compatible
> blobs.
>
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Signed-off-by: Jan Luebbe <jlu-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
> Documentation/manual.txt | 29 ++++++-
> checks.c | 8 +-
> dtc-lexer.l | 5 ++
> dtc-parser.y | 29 ++++++-
> dtc.c | 51 +++++++++++-
> dtc.h | 21 ++++-
> fdtdump.c | 2 +-
> flattree.c | 17 ++--
> fstree.c | 2 +-
> libfdt/fdt.c | 2 +-
> libfdt/fdt.h | 3 +-
> livetree.c | 208 ++++++++++++++++++++++++++++++++++++++++++++++-
> tests/mangle-layout.c | 7 +-
> 13 files changed, 357 insertions(+), 27 deletions(-)
>
> diff --git a/Documentation/manual.txt b/Documentation/manual.txt
> index 398de32..92a4966 100644
> --- a/Documentation/manual.txt
> +++ b/Documentation/manual.txt
> @@ -119,6 +119,28 @@ Options:
> Make space for <number> reserve map entries
> Relevant for dtb and asm output only.
>
> + -@
> + Generates a __symbols__ node at the root node of the resulting blob
> + for any node labels used, and for any local references using phandles
> + it also generates a __local_fixups__ node that tracks them.
> +
> + When using the /plugin/ tag all unresolved label references to
> + be tracked in the __fixups__ node, making dynamic resolution possible.
> +
> + -A
> + Generate automatically aliases for all node labels. This is similar to
> + the -@ option (the __symbols__ node contain identical information) but
> + the semantics are slightly different since no phandles are automatically
> + generated for labeled nodes.
> +
> + -M
> + Generate blobs with the old FDT magic number for device tree objects.
> + By default blobs use the DTBO FDT magic number instead.
> +
> + -F
> + Suppress generation of fixups when -@ is used. This is useful for generating
> + a base tree with symbols but without fixups that take some amount of space.
Urgh, yet another option?
Can you give me more details on how __fixups__ is useful in a base tree?
> -S <bytes>
> Ensure the blob at least <bytes> long, adding additional
> space if needed.
> @@ -146,13 +168,18 @@ Additionally, dtc performs various sanity checks on the tree.
> Here is a very rough overview of the layout of a DTS source file:
>
>
> - sourcefile: list_of_memreserve devicetree
> + sourcefile: versioninfo plugindecl list_of_memreserve devicetree
>
> memreserve: label 'memreserve' ADDR ADDR ';'
> | label 'memreserve' ADDR '-' ADDR ';'
>
> devicetree: '/' nodedef
>
> + versioninfo: '/' 'dts-v1' '/' ';'
> +
> + plugindecl: '/' 'plugin' '/' ';'
> + | /* empty */
> +
> nodedef: '{' list_of_property list_of_subnode '}' ';'
>
> property: label PROPNAME '=' propdata ';'
> diff --git a/checks.c b/checks.c
> index 2bd27a4..4292f4b 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -487,8 +487,12 @@ static void fixup_phandle_references(struct check *c, struct boot_info *bi,
>
> refnode = get_node_by_ref(dt, m->ref);
> if (! refnode) {
> - FAIL(c, "Reference to non-existent node or label \"%s\"\n",
> - m->ref);
> + if (!(bi->versionflags & VF_PLUGIN))
> + FAIL(c, "Reference to non-existent node or "
> + "label \"%s\"\n", m->ref);
> + else /* mark the entry as unresolved */
> + *((cell_t *)(prop->val.val + m->offset)) =
> + cpu_to_fdt32(0xffffffff);
> continue;
> }
>
> diff --git a/dtc-lexer.l b/dtc-lexer.l
> index 790fbf6..40bbc87 100644
> --- a/dtc-lexer.l
> +++ b/dtc-lexer.l
> @@ -121,6 +121,11 @@ static void lexical_error(const char *fmt, ...);
> return DT_V1;
> }
>
> +<*>"/plugin/" {
> + DPRINT("Keyword: /plugin/\n");
> + return DT_PLUGIN;
> + }
> +
> <*>"/memreserve/" {
> DPRINT("Keyword: /memreserve/\n");
> BEGIN_DEFAULT();
> diff --git a/dtc-parser.y b/dtc-parser.y
> index 14aaf2e..ad3dbe2 100644
> --- a/dtc-parser.y
> +++ b/dtc-parser.y
> @@ -19,6 +19,7 @@
> */
> %{
> #include <stdio.h>
> +#include <inttypes.h>
>
> #include "dtc.h"
> #include "srcpos.h"
> @@ -52,9 +53,11 @@ extern bool treesource_error;
> struct node *nodelist;
> struct reserve_info *re;
> uint64_t integer;
> + unsigned int flags;
> }
>
> %token DT_V1
> +%token DT_PLUGIN
> %token DT_MEMRESERVE
> %token DT_LSHIFT DT_RSHIFT DT_LE DT_GE DT_EQ DT_NE DT_AND DT_OR
> %token DT_BITS
> @@ -71,6 +74,8 @@ extern bool treesource_error;
>
> %type <data> propdata
> %type <data> propdataprefix
> +%type <flags> versioninfo
> +%type <flags> plugindecl
> %type <re> memreserve
> %type <re> memreserves
> %type <array> arrayprefix
> @@ -101,16 +106,34 @@ extern bool treesource_error;
> %%
>
> sourcefile:
> - v1tag memreserves devicetree
> + versioninfo plugindecl memreserves devicetree
> {
> - the_boot_info = build_boot_info($2, $3,
> - guess_boot_cpuid($3));
> + the_boot_info = build_boot_info($1 | $2, $3, $4,
> + guess_boot_cpuid($4));
> + }
> + ;
> +
> +versioninfo:
> + v1tag
> + {
> + $$ = VF_DT_V1;
> }
> ;
>
> v1tag:
> DT_V1 ';'
> + | DT_V1
> | DT_V1 ';' v1tag
> +
> +plugindecl:
> + DT_PLUGIN ';'
> + {
> + $$ = VF_PLUGIN;
> + }
> + | /* empty */
> + {
> + $$ = 0;
> + }
> ;
>
> memreserves:
> diff --git a/dtc.c b/dtc.c
> index 9dcf640..947d082 100644
> --- a/dtc.c
> +++ b/dtc.c
> @@ -32,6 +32,10 @@ int minsize; /* Minimum blob size */
> int padsize; /* Additional padding to blob */
> int alignsize; /* Additional padding to blob accroding to the alignsize */
> int phandle_format = PHANDLE_BOTH; /* Use linux,phandle or phandle properties */
> +int symbol_fixup_support; /* enable symbols & fixup support */
> +int auto_label_aliases; /* auto generate labels -> aliases */
> +int no_dtbo_magic; /* use old FDT magic values for objects */
> +int suppress_fixups; /* suppress generation of fixups on symbol support */
The symbol_fixup_support and suppress_fixups flags semantics are
starting to confuse me. I think rework these to 'generate_symbols'
and 'generate_fixups' which should have clearer logic.
Note that - for now - I'm just suggesting a change in the internal
variables, they can still be set appropriately based on the existing
semantics of the command line options.
> static int is_power_of_2(int x)
> {
> @@ -59,7 +63,7 @@ static void fill_fullpaths(struct node *tree, const char *prefix)
> #define FDT_VERSION(version) _FDT_VERSION(version)
> #define _FDT_VERSION(version) #version
> static const char usage_synopsis[] = "dtc [options] <input file>";
> -static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:a:fb:i:H:sW:E:hv";
> +static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:a:fb:i:H:sW:E:@AMFhv";
> static struct option const usage_long_opts[] = {
> {"quiet", no_argument, NULL, 'q'},
> {"in-format", a_argument, NULL, 'I'},
> @@ -78,6 +82,10 @@ static struct option const usage_long_opts[] = {
> {"phandle", a_argument, NULL, 'H'},
> {"warning", a_argument, NULL, 'W'},
> {"error", a_argument, NULL, 'E'},
> + {"symbols", no_argument, NULL, '@'},
> + {"auto-alias", no_argument, NULL, 'A'},
> + {"no-dtbo-magic", no_argument, NULL, 'M'},
> + {"suppress-fixups", no_argument, NULL, 'F'},
> {"help", no_argument, NULL, 'h'},
> {"version", no_argument, NULL, 'v'},
> {NULL, no_argument, NULL, 0x0},
> @@ -109,6 +117,10 @@ static const char * const usage_opts_help[] = {
> "\t\tboth - Both \"linux,phandle\" and \"phandle\" properties",
> "\n\tEnable/disable warnings (prefix with \"no-\")",
> "\n\tEnable/disable errors (prefix with \"no-\")",
> + "\n\tEnable symbols/fixup support",
> + "\n\tEnable auto-alias of labels",
> + "\n\tDo not use DTBO magic value for plugin objects",
> + "\n\tSuppress generation of fixups when symbol support enabled",
> "\n\tPrint this help and exit",
> "\n\tPrint version and exit",
> NULL,
> @@ -153,7 +165,7 @@ static const char *guess_input_format(const char *fname, const char *fallback)
> fclose(f);
>
> magic = fdt32_to_cpu(magic);
> - if (magic == FDT_MAGIC)
> + if (magic == FDT_MAGIC || magic == FDT_MAGIC_DTBO)
> return "dtb";
>
> return guess_type_by_name(fname, fallback);
> @@ -172,6 +184,7 @@ int main(int argc, char *argv[])
> FILE *outf = NULL;
> int outversion = DEFAULT_FDT_VERSION;
> long long cmdline_boot_cpuid = -1;
> + fdt32_t out_magic = FDT_MAGIC;
>
> quiet = 0;
> reservenum = 0;
> @@ -249,6 +262,19 @@ int main(int argc, char *argv[])
> parse_checks_option(false, true, optarg);
> break;
>
> + case '@':
> + symbol_fixup_support = 1;
> + break;
> + case 'A':
> + auto_label_aliases = 1;
> + break;
> + case 'M':
> + no_dtbo_magic = 1;
> + break;
> + case 'F':
> + suppress_fixups = 1;
> + break;
> +
> case 'h':
> usage(NULL);
> default:
> @@ -306,6 +332,20 @@ int main(int argc, char *argv[])
> fill_fullpaths(bi->dt, "");
> process_checks(force, bi);
>
> + if (auto_label_aliases)
> + generate_label_tree(bi, "aliases", false);
> +
> + /* symbol support or plugin is detected */
> + if (symbol_fixup_support || (bi->versionflags & VF_PLUGIN))
> + generate_label_tree(bi, "__symbols__", true);
> +
> + /* plugin or symbol support and fixups are not suppressed */
> + if ((bi->versionflags & VF_PLUGIN) ||
> + (symbol_fixup_support && !suppress_fixups)) {
> + generate_fixups_tree(bi, "__fixups__");
> + generate_local_fixups_tree(bi, "__local_fixups__");
> + }
Elaborating on the suggestion above, I'd like to see this just be:
if (generate_symbols)
generate_label_tree(...);
if (generate_fixups) {
generate_fixups_tree(..);
generate_local_fixups_tree(...);
}
With the two option flags set earlier according to the command line
options + /plugin/ tag.
> if (sort)
> sort_tree(bi);
>
> @@ -318,12 +358,15 @@ int main(int argc, char *argv[])
> outname, strerror(errno));
> }
>
> + if (!no_dtbo_magic && (bi->versionflags & VF_PLUGIN))
> + out_magic = FDT_MAGIC_DTBO;
> +
> if (streq(outform, "dts")) {
> dt_to_source(outf, bi);
> } else if (streq(outform, "dtb")) {
> - dt_to_blob(outf, bi, outversion);
> + dt_to_blob(outf, bi, out_magic, outversion);
> } else if (streq(outform, "asm")) {
> - dt_to_asm(outf, bi, outversion);
> + dt_to_asm(outf, bi, out_magic, outversion);
> } else if (streq(outform, "null")) {
> /* do nothing */
> } else {
> diff --git a/dtc.h b/dtc.h
> index 32009bc..4da5a37 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -55,6 +55,10 @@ extern int minsize; /* Minimum blob size */
> extern int padsize; /* Additional padding to blob */
> extern int alignsize; /* Additional padding to blob accroding to the alignsize */
> extern int phandle_format; /* Use linux,phandle or phandle properties */
> +extern int symbol_fixup_support;/* enable symbols & fixup support */
> +extern int auto_label_aliases; /* auto generate labels -> aliases */
> +extern int no_dtbo_magic; /* use old FDT magic values for objects */
> +extern int suppress_fixups; /* suppress generation of fixups on symbol support */
>
> #define PHANDLE_LEGACY 0x1
> #define PHANDLE_EPAPR 0x2
> @@ -202,6 +206,8 @@ void delete_property(struct property *prop);
> void add_child(struct node *parent, struct node *child);
> void delete_node_by_name(struct node *parent, char *name);
> void delete_node(struct node *node);
> +void append_to_property(struct node *node,
> + char *name, const void *data, int len);
>
> const char *get_unitname(struct node *node);
> struct property *get_property(struct node *node, const char *propname);
> @@ -237,14 +243,23 @@ struct reserve_info *add_reserve_entry(struct reserve_info *list,
>
>
> struct boot_info {
> + unsigned int versionflags;
> struct reserve_info *reservelist;
> struct node *dt; /* the device tree */
> uint32_t boot_cpuid_phys;
> };
>
> -struct boot_info *build_boot_info(struct reserve_info *reservelist,
> +/* version flags definitions */
> +#define VF_DT_V1 0x0001 /* /dts-v1/ */
> +#define VF_PLUGIN 0x0002 /* /plugin/ */
Hm, sorry, didn't mention this minor nit before. Can you rename
versionflags and the associated constants to make it clear these are
about the *dts* version, since we also have entirely different
versioning info for the *dtb* version.
> +struct boot_info *build_boot_info(unsigned int versionflags,
> + struct reserve_info *reservelist,
> struct node *tree, uint32_t boot_cpuid_phys);
> void sort_tree(struct boot_info *bi);
> +void generate_label_tree(struct boot_info *bi, char *name, bool allocph);
> +void generate_fixups_tree(struct boot_info *bi, char *name);
> +void generate_local_fixups_tree(struct boot_info *bi, char *name);
>
> /* Checks */
>
> @@ -253,8 +268,8 @@ void process_checks(bool force, struct boot_info *bi);
>
> /* Flattened trees */
>
> -void dt_to_blob(FILE *f, struct boot_info *bi, int version);
> -void dt_to_asm(FILE *f, struct boot_info *bi, int version);
> +void dt_to_blob(FILE *f, struct boot_info *bi, fdt32_t magic, int version);
> +void dt_to_asm(FILE *f, struct boot_info *bi, fdt32_t magic, int version);
>
> struct boot_info *dt_from_blob(const char *fname);
>
> diff --git a/fdtdump.c b/fdtdump.c
> index a9a2484..dd63ac2 100644
> --- a/fdtdump.c
> +++ b/fdtdump.c
> @@ -201,7 +201,7 @@ int main(int argc, char *argv[])
> p = memchr(p, smagic[0], endp - p - FDT_MAGIC_SIZE);
> if (!p)
> break;
> - if (fdt_magic(p) == FDT_MAGIC) {
> + if (fdt_magic(p) == FDT_MAGIC || fdt_magic(p) == FDT_MAGIC_DTBO) {
> /* try and validate the main struct */
> off_t this_len = endp - p;
> fdt32_t max_version = 17;
> diff --git a/flattree.c b/flattree.c
> index a9d9520..57d76cf 100644
> --- a/flattree.c
> +++ b/flattree.c
> @@ -335,6 +335,7 @@ static struct data flatten_reserve_list(struct reserve_info *reservelist,
> }
>
> static void make_fdt_header(struct fdt_header *fdt,
> + fdt32_t magic,
> struct version_info *vi,
> int reservesize, int dtsize, int strsize,
> int boot_cpuid_phys)
> @@ -345,7 +346,7 @@ static void make_fdt_header(struct fdt_header *fdt,
>
> memset(fdt, 0xff, sizeof(*fdt));
>
> - fdt->magic = cpu_to_fdt32(FDT_MAGIC);
> + fdt->magic = cpu_to_fdt32(magic);
> fdt->version = cpu_to_fdt32(vi->version);
> fdt->last_comp_version = cpu_to_fdt32(vi->last_comp_version);
>
> @@ -366,7 +367,7 @@ static void make_fdt_header(struct fdt_header *fdt,
> fdt->size_dt_struct = cpu_to_fdt32(dtsize);
> }
>
> -void dt_to_blob(FILE *f, struct boot_info *bi, int version)
> +void dt_to_blob(FILE *f, struct boot_info *bi, fdt32_t magic, int version)
> {
> struct version_info *vi = NULL;
> int i;
> @@ -390,7 +391,7 @@ void dt_to_blob(FILE *f, struct boot_info *bi, int version)
> reservebuf = flatten_reserve_list(bi->reservelist, vi);
>
> /* Make header */
> - make_fdt_header(&fdt, vi, reservebuf.len, dtbuf.len, strbuf.len,
> + make_fdt_header(&fdt, magic, vi, reservebuf.len, dtbuf.len, strbuf.len,
> bi->boot_cpuid_phys);
>
> /*
> @@ -467,7 +468,7 @@ static void dump_stringtable_asm(FILE *f, struct data strbuf)
> }
> }
>
> -void dt_to_asm(FILE *f, struct boot_info *bi, int version)
> +void dt_to_asm(FILE *f, struct boot_info *bi, fdt32_t magic, int version)
> {
> struct version_info *vi = NULL;
> int i;
> @@ -830,6 +831,7 @@ struct boot_info *dt_from_blob(const char *fname)
> struct node *tree;
> uint32_t val;
> int flags = 0;
> + unsigned int versionflags = VF_DT_V1;
Especially here calling this just 'versionflags' is potentially
confusing, since it's not related to the dtb version that we're also
dealing with in this vicinity.
> f = srcfile_relative_open(fname, NULL);
>
> @@ -845,9 +847,12 @@ struct boot_info *dt_from_blob(const char *fname)
> }
>
> magic = fdt32_to_cpu(magic);
> - if (magic != FDT_MAGIC)
> + if (magic != FDT_MAGIC && magic != FDT_MAGIC_DTBO)
> die("Blob has incorrect magic number\n");
>
> + if (magic == FDT_MAGIC_DTBO)
> + versionflags |= VF_PLUGIN;
> +
> rc = fread(&totalsize, sizeof(totalsize), 1, f);
> if (ferror(f))
> die("Error reading DT blob size: %s\n", strerror(errno));
> @@ -942,5 +947,5 @@ struct boot_info *dt_from_blob(const char *fname)
>
> fclose(f);
>
> - return build_boot_info(reservelist, tree, boot_cpuid_phys);
> + return build_boot_info(versionflags, reservelist, tree, boot_cpuid_phys);
> }
> diff --git a/fstree.c b/fstree.c
> index 6d1beec..54f520b 100644
> --- a/fstree.c
> +++ b/fstree.c
> @@ -86,6 +86,6 @@ struct boot_info *dt_from_fs(const char *dirname)
> tree = read_fstree(dirname);
> tree = name_node(tree, "");
>
> - return build_boot_info(NULL, tree, guess_boot_cpuid(tree));
> + return build_boot_info(VF_DT_V1, NULL, tree, guess_boot_cpuid(tree));
> }
>
> diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> index 22286a1..28d422c 100644
> --- a/libfdt/fdt.c
> +++ b/libfdt/fdt.c
> @@ -57,7 +57,7 @@
>
> int fdt_check_header(const void *fdt)
> {
> - if (fdt_magic(fdt) == FDT_MAGIC) {
> + if (fdt_magic(fdt) == FDT_MAGIC || fdt_magic(fdt) == FDT_MAGIC_DTBO) {
> /* Complete tree */
> if (fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
> return -FDT_ERR_BADVERSION;
> diff --git a/libfdt/fdt.h b/libfdt/fdt.h
> index 526aedb..493cd55 100644
> --- a/libfdt/fdt.h
> +++ b/libfdt/fdt.h
> @@ -55,7 +55,7 @@
> #ifndef __ASSEMBLY__
>
> struct fdt_header {
> - fdt32_t magic; /* magic word FDT_MAGIC */
> + fdt32_t magic; /* magic word FDT_MAGIC[|_DTBO] */
> fdt32_t totalsize; /* total size of DT block */
> fdt32_t off_dt_struct; /* offset to structure */
> fdt32_t off_dt_strings; /* offset to strings */
> @@ -93,6 +93,7 @@ struct fdt_property {
> #endif /* !__ASSEMBLY */
>
> #define FDT_MAGIC 0xd00dfeed /* 4: version, 4: total size */
> +#define FDT_MAGIC_DTBO 0xd00dfdb0 /* DTBO magic */
> #define FDT_TAGSIZE sizeof(fdt32_t)
>
> #define FDT_BEGIN_NODE 0x1 /* Start node: full name */
> diff --git a/livetree.c b/livetree.c
> index 3dc7559..17f8310 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -296,6 +296,23 @@ void delete_node(struct node *node)
> delete_labels(&node->labels);
> }
>
> +void append_to_property(struct node *node,
> + char *name, const void *data, int len)
> +{
> + struct data d;
> + struct property *p;
> +
> + p = get_property(node, name);
> + if (p) {
> + d = data_append_data(p->val, data, len);
> + p->val = d;
> + } else {
> + d = data_append_data(empty_data, data, len);
> + p = build_property(name, d);
> + add_property(node, p);
> + }
> +}
> +
> struct reserve_info *build_reserve_entry(uint64_t address, uint64_t size)
> {
> struct reserve_info *new = xmalloc(sizeof(*new));
> @@ -335,12 +352,14 @@ struct reserve_info *add_reserve_entry(struct reserve_info *list,
> return list;
> }
>
> -struct boot_info *build_boot_info(struct reserve_info *reservelist,
> +struct boot_info *build_boot_info(unsigned int versionflags,
> + struct reserve_info *reservelist,
> struct node *tree, uint32_t boot_cpuid_phys)
> {
> struct boot_info *bi;
>
> bi = xmalloc(sizeof(*bi));
> + bi->versionflags = versionflags;
> bi->reservelist = reservelist;
> bi->dt = tree;
> bi->boot_cpuid_phys = boot_cpuid_phys;
> @@ -709,3 +728,190 @@ void sort_tree(struct boot_info *bi)
> sort_reserve_entries(bi);
> sort_node(bi->dt);
> }
> +
> +/* utility helper to avoid code duplication */
> +static struct node *build_and_name_child_node(struct node *parent, char *name)
> +{
> + struct node *node;
> +
> + node = build_node(NULL, NULL);
> + name_node(node, xstrdup(name));
> + add_child(parent, node);
> +
> + return node;
> +}
> +
> +static void generate_label_tree_internal(struct boot_info *bi,
> + struct node *an,
> + struct node *node,
> + bool allocph)
> +{
> + struct node *dt = bi->dt;
> + struct node *c;
> + struct property *p;
> + struct label *l;
> +
> + /* if if there are labels */
> + if (node->labels) {
> + /* now add the label in the node */
> + for_each_label(node->labels, l) {
> + /* check whether the label already exists */
> + p = get_property(an, l->label);
> + if (p) {
> + fprintf(stderr, "WARNING: label %s already"
> + " exists in /%s", l->label,
> + an->name);
> + continue;
> + }
> +
> + /* insert it */
> + p = build_property(l->label,
> + data_copy_mem(node->fullpath,
> + strlen(node->fullpath) + 1));
> + add_property(an, p);
> + }
> +
> + /* force allocation of a phandle for this node */
> + if (allocph)
> + (void)get_node_phandle(dt, node);
> + }
> +
> + for_each_child(node, c)
> + generate_label_tree_internal(bi, an, c, allocph);
> +}
> +
> +static void add_fixup_entry(struct boot_info *bi, struct node *fn,
> + struct node *node, struct property *prop,
> + struct marker *m)
> +{
> + char *entry;
> +
> + /* m->ref can only be a REF_PHANDLE, but check anyway */
> + assert(m->type == REF_PHANDLE);
> +
> + /* there shouldn't be any ':' in the arguments */
> + if (strchr(node->fullpath, ':') || strchr(prop->name, ':'))
> + die("arguments should not contain ':'\n");
> +
> + xasprintf(&entry, "%s:%s:%u",
> + node->fullpath, prop->name, m->offset);
> + append_to_property(fn, m->ref, entry, strlen(entry) + 1);
> +}
> +
> +static void generate_fixups_tree_internal(struct boot_info *bi,
> + struct node *fn,
> + struct node *node)
> +{
> + struct node *dt = bi->dt;
> + struct node *c;
> + struct property *prop;
> + struct marker *m;
> + struct node *refnode;
> +
> + for_each_property(node, prop) {
> + m = prop->val.markers;
> + for_each_marker_of_type(m, REF_PHANDLE) {
> + refnode = get_node_by_ref(dt, m->ref);
> + if (!refnode)
> + add_fixup_entry(bi, fn, node, prop, m);
> + }
> + }
> +
> + for_each_child(node, c)
> + generate_fixups_tree_internal(bi, fn, c);
> +}
> +
> +static void add_local_fixup_entry(struct boot_info *bi,
> + struct node *lfn, struct node *node,
> + struct property *prop, struct marker *m,
> + struct node *refnode)
> +{
> + struct node *wn, *nwn; /* local fixup node, walk node, new */
> + uint32_t value_32;
> + char **compp;
> + int i, depth;
> +
> + /* walk back retreiving depth */
> + depth = 0;
> + for (wn = node; wn; wn = wn->parent)
> + depth++;
> +
> + /* allocate name array */
> + compp = xmalloc(sizeof(*compp) * depth);
> +
> + /* store names in the array */
> + for (wn = node, i = depth - 1; wn; wn = wn->parent, i--)
> + compp[i] = wn->name;
> +
> + /* walk the path components creating nodes if they don't exist */
> + for (wn = lfn, i = 1; i < depth; i++, wn = nwn) {
> + /* if no node exists, create it */
> + nwn = get_subnode(wn, compp[i]);
> + if (!nwn)
> + nwn = build_and_name_child_node(wn, compp[i]);
> + }
> +
> + free(compp);
> +
> + value_32 = cpu_to_fdt32(m->offset);
> + append_to_property(wn, prop->name, &value_32, sizeof(value_32));
> +}
> +
> +static void generate_local_fixups_tree_internal(struct boot_info *bi,
> + struct node *lfn,
> + struct node *node)
> +{
> + struct node *dt = bi->dt;
> + struct node *c;
> + struct property *prop;
> + struct marker *m;
> + struct node *refnode;
> +
> + for_each_property(node, prop) {
> + m = prop->val.markers;
> + for_each_marker_of_type(m, REF_PHANDLE) {
> + refnode = get_node_by_ref(dt, m->ref);
> + if (!refnode)
> + continue;
> + add_local_fixup_entry(bi, lfn, node, prop, m, refnode);
> + }
> + }
> +
> + for_each_child(node, c)
> + generate_local_fixups_tree_internal(bi, lfn, c);
> +}
> +
> +static struct node *build_root_node(struct node *dt, char *name)
> +{
> + struct node *an;
> +
> + for_each_child(dt, an)
> + if (streq(name, an->name))
> + break;
> +
> + if (!an)
> + an = build_and_name_child_node(dt, name);
> +
> + if (!an)
> + die("Could not build root node /%s\n", name);
> +
> + return an;
> +}
> +
> +void generate_label_tree(struct boot_info *bi, char *name, bool allocph)
> +{
> + generate_label_tree_internal(bi, build_root_node(bi->dt, name),
> + bi->dt, allocph);
> +}
> +
> +void generate_fixups_tree(struct boot_info *bi, char *name)
> +{
> + generate_fixups_tree_internal(bi, build_root_node(bi->dt, name),
> + bi->dt);
> +}
> +
> +void generate_local_fixups_tree(struct boot_info *bi, char *name)
> +{
> + generate_local_fixups_tree_internal(bi, build_root_node(bi->dt, name),
> + bi->dt);
> +}
> diff --git a/tests/mangle-layout.c b/tests/mangle-layout.c
> index a76e51e..d29ebc6 100644
> --- a/tests/mangle-layout.c
> +++ b/tests/mangle-layout.c
> @@ -42,7 +42,8 @@ static void expand_buf(struct bufstate *buf, int newsize)
> buf->size = newsize;
> }
>
> -static void new_header(struct bufstate *buf, int version, const void *fdt)
> +static void new_header(struct bufstate *buf, fdt32_t magic, int version,
> + const void *fdt)
> {
> int hdrsize;
>
> @@ -56,7 +57,7 @@ static void new_header(struct bufstate *buf, int version, const void *fdt)
> expand_buf(buf, hdrsize);
> memset(buf->buf, 0, hdrsize);
>
> - fdt_set_magic(buf->buf, FDT_MAGIC);
> + fdt_set_magic(buf->buf, magic);
> fdt_set_version(buf->buf, version);
> fdt_set_last_comp_version(buf->buf, 16);
> fdt_set_boot_cpuid_phys(buf->buf, fdt_boot_cpuid_phys(fdt));
> @@ -145,7 +146,7 @@ int main(int argc, char *argv[])
> if (fdt_version(fdt) < 17)
> CONFIG("Input tree must be v17");
>
> - new_header(&buf, version, fdt);
> + new_header(&buf, FDT_MAGIC, version, fdt);
>
> while (*blockorder) {
> add_block(&buf, version, *blockorder, fdt);
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v11 3/7] tests: Add check_path test
From: David Gibson @ 2016-11-29 2:40 UTC (permalink / raw)
To: Pantelis Antoniou
Cc: Jon Loeliger, Grant Likely, Frank Rowand, Rob Herring, Jan Luebbe,
Sascha Hauer, Phil Elwell, Simon Glass, Maxime Ripard,
Thomas Petazzoni, Boris Brezillon, Antoine Tenart, Stephen Boyd,
Devicetree Compiler, devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1480349141-14145-4-git-send-email-pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 4113 bytes --]
On Mon, Nov 28, 2016 at 06:05:37PM +0200, Pantelis Antoniou wrote:
> Add a test that checks for existence or not of a node.
> It is useful for testing the various cases when generating
> symbols and fixups for dynamic device tree objects.
>
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
Can you please add a test-for-the-test by putting a couple of
invocations of this on test_tree1 into the runner script (one 'exists'
and one 'not-exists' should suffice).
> ---
> tests/.gitignore | 1 +
> tests/Makefile.tests | 3 +-
> tests/check_path.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 85 insertions(+), 1 deletion(-)
> create mode 100644 tests/check_path.c
>
> diff --git a/tests/.gitignore b/tests/.gitignore
> index 354b565..9e209d5 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -8,6 +8,7 @@ tmp.*
> /asm_tree_dump
> /boot-cpuid
> /char_literal
> +/check_path
> /del_node
> /del_property
> /dtbs_equal_ordered
> diff --git a/tests/Makefile.tests b/tests/Makefile.tests
> index eb039c5..3d7a4f8 100644
> --- a/tests/Makefile.tests
> +++ b/tests/Makefile.tests
> @@ -25,7 +25,8 @@ LIB_TESTS_L = get_mem_rsv \
> integer-expressions \
> property_iterate \
> subnode_iterate \
> - overlay overlay_bad_fixup
> + overlay overlay_bad_fixup \
> + check_path
> LIB_TESTS = $(LIB_TESTS_L:%=$(TESTS_PREFIX)%)
>
> LIBTREE_TESTS_L = truncated_property
> diff --git a/tests/check_path.c b/tests/check_path.c
> new file mode 100644
> index 0000000..0d6a73b
> --- /dev/null
> +++ b/tests/check_path.c
> @@ -0,0 +1,82 @@
> +/*
> + * libfdt - Flat Device Tree manipulation
> + * Testcase for node existence
> + * Copyright (C) 2016 Konsulko Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public License
> + * as published by the Free Software Foundation; either version 2.1 of
> + * the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include <stdio.h>
> +
> +#include <libfdt.h>
> +
> +#include "tests.h"
> +
> +#define CHECK(code) \
> + { \
> + if (code) \
> + FAIL(#code ": %s", fdt_strerror(code)); \
> + }
> +
> +/* 4k ought to be enough for anybody */
> +#define FDT_COPY_SIZE (4 * 1024)
> +
> +static void *open_dt(char *path)
> +{
> + void *dt, *copy;
> +
> + dt = load_blob(path);
> + copy = xmalloc(FDT_COPY_SIZE);
> +
> + /*
> + * Resize our DTs to 4k so that we have room to operate on
> + */
> + CHECK(fdt_open_into(dt, copy, FDT_COPY_SIZE));
> +
> + return copy;
> +}
> +
> +int main(int argc, char *argv[])
> +{
> + void *fdt_base;
> + int fail_config, exists, check_exists;
> +
> + test_init(argc, argv);
> + fail_config = 0;
> +
> + if (argc != 4)
> + fail_config = 1;
> +
> + if (!fail_config) {
> + if (!strcmp(argv[2], "exists"))
> + check_exists = 1;
> + else if (!strcmp(argv[2], "not-exists"))
> + check_exists = 0;
> + else
> + fail_config = 1;
> + }
> +
> + if (fail_config)
> + CONFIG("Usage: %s <base dtb> <[exists|not-exists]> <node-path>", argv[0]);
> +
> + fdt_base = open_dt(argv[1]);
> +
> + exists = fdt_path_offset(fdt_base, argv[3]) >= 0;
> +
> + if (exists == check_exists)
> + PASS();
> + else
> + FAIL();
> +}
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC
From: Ziji Hu @ 2016-11-29 2:53 UTC (permalink / raw)
To: Ulf Hansson
Cc: Gregory CLEMENT, Adrian Hunter, linux-mmc@vger.kernel.org,
Jason Cooper, Andrew Lunn, Sebastian Hesselbarth, Rob Herring,
devicetree@vger.kernel.org, Thomas Petazzoni,
linux-arm-kernel@lists.infradead.org, Jimmy Xu, Jisheng Zhang,
Nadav Haklai, Ryan Gao, Doug Jones, Victor Gu, Wei(SOCP) Liu,
Wilson Ding
In-Reply-To: <CAPDyKFr8rX04iY92OeQpSkS+3HN2-FxijCCiDb2sSKvP+TZPog@mail.gmail.com>
Hi Ulf,
On 2016/11/28 23:16, Ulf Hansson wrote:
> On 28 November 2016 at 12:38, Ziji Hu <huziji@marvell.com> wrote:
>> Hi Ulf,
>>
>> On 2016/11/28 19:13, Ulf Hansson wrote:
>>>>
>>>> As you suggest, I replace mmc_wait_for_cmd() with mmc_send_tuning(), to
>>>> send commands for testing current sampling point set in our host PHY.
>>>>
>>>> According to my test result, it shows that mmc_send_tuning() can only support
>>>> tuning command (CMD21/CMD19).
>>>> As a result, we cannot use mmc_send_tuning() when card is in the speed modes
>>>> which doesn't support tuning, such as eMMC HS SDR, eMMC HS DRR and
>>>> SD SDR 12/SDR25/DDR50. Card will not response to tuning commands in those
>>>> speed modes.
>>>>
>>>> Could you please provide suggestions for the speed mode in which tuning is
>>>> not available?
>>>>
>>>
>>> Normally the mmc host driver shouldn't have to care about what the
>>> card supports, as that is the responsibility of the mmc core to
>>> manage.
>>>
>>> The host should only need to implement the ->execute_tuning() ops,
>>> which gets called when the card supports tuning (CMD19/21). Does it
>>> make sense?
>>>
>> I think it is irrelevant to tuning procedure.
>>
>> Our host requires to adjust PHY setting after each time ios setting
>> (SDCLK/bus width/speed mode) is changed.
>> The simplified sequence is:
>> mmc change ios --> mmc_set_ios() --> ->set_ios() --> after sdhci_set_ios(),
>> adjust PHY setting.
>> During PHY setting adjustment, out host driver has to send commands to
>> test current sampling point. Tuning is another independent step.
>
> For those speed modes (or other ios changes) that *don't* requires
> tuning, then what will you do when you send the command to confirm the
> change of PHY setting and it fails?
>
> My assumption is that you will fail anyway, by propagating the error
> to the mmc core. At least that what was my understanding from your
> earlier replies, right!?
>
> Then, I think there are no point having the host driver sending a
> command to confirm the PHY settings, as the mmc core will anyway
> discover if something goes wrong when the next command is sent.
>
> Please correct me if I am wrong!
>
Sorry that I didn't make myself clear.
Our host PHY delay line consists of hundreds of sampling points.
Each sampling point represents a different phase shift.
In lower speed mode, our host driver will scan the delay line.
It will select and test multiple sampling points, other than testing
only single sampling point.
If a sampling point fails to transfer cmd/data, our host driver will
move to test next sampling point, until we find out a group of successful
sampling points which can transfer cmd/data. At last we will select
a perfect one from them.
Thank you.
Best regards,
Hu Ziji
>>
>> Thus our host needs a valid command in PHY setting adjustment. Tuning command
>> can be borrowed to complete this task in SD SDR50. But for other speed mode,
>> we have to find out a valid command.
>
> I thought we agreed on this wasn't necessary? Please see my upper response.
>
> Kind regards
> Uffe
>
^ permalink raw reply
* Re: [PATCH v11 4/7] tests: Add overlay tests
From: David Gibson @ 2016-11-29 3:08 UTC (permalink / raw)
To: Pantelis Antoniou
Cc: Jon Loeliger, Grant Likely, Frank Rowand, Rob Herring, Jan Luebbe,
Sascha Hauer, Phil Elwell, Simon Glass, Maxime Ripard,
Thomas Petazzoni, Boris Brezillon, Antoine Tenart, Stephen Boyd,
Devicetree Compiler, devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1480349141-14145-5-git-send-email-pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 9292 bytes --]
On Mon, Nov 28, 2016 at 06:05:38PM +0200, Pantelis Antoniou wrote:
> Add a number of tests for dynamic objects/overlays.
>
> Re-use the original test by moving the contents to a .dtsi include
>
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> ---
> tests/overlay_overlay_dtc.dts | 76 +----------------------------------
> tests/overlay_overlay_dtc.dtsi | 83 +++++++++++++++++++++++++++++++++++++++
> tests/overlay_overlay_new_dtc.dts | 11 ++++++
> tests/overlay_overlay_simple.dts | 12 ++++++
> tests/run_tests.sh | 41 +++++++++++++++++++
> 5 files changed, 148 insertions(+), 75 deletions(-)
> create mode 100644 tests/overlay_overlay_dtc.dtsi
> create mode 100644 tests/overlay_overlay_new_dtc.dts
> create mode 100644 tests/overlay_overlay_simple.dts
>
> diff --git a/tests/overlay_overlay_dtc.dts b/tests/overlay_overlay_dtc.dts
> index 30d2362..ca943ea 100644
> --- a/tests/overlay_overlay_dtc.dts
> +++ b/tests/overlay_overlay_dtc.dts
> @@ -8,78 +8,4 @@
> /dts-v1/;
> /plugin/;
>
> -/ {
> - /* Test that we can change an int by another */
> - fragment@0 {
> - target = <&test>;
> -
> - __overlay__ {
> - test-int-property = <43>;
> - };
> - };
> -
> - /* Test that we can replace a string by a longer one */
> - fragment@1 {
> - target = <&test>;
> -
> - __overlay__ {
> - test-str-property = "foobar";
> - };
> - };
> -
> - /* Test that we add a new property */
> - fragment@2 {
> - target = <&test>;
> -
> - __overlay__ {
> - test-str-property-2 = "foobar2";
> - };
> - };
> -
> - /* Test that we add a new node (by phandle) */
> - fragment@3 {
> - target = <&test>;
> -
> - __overlay__ {
> - new-node {
> - new-property;
> - };
> - };
> - };
> -
> - fragment@5 {
> - target = <&test>;
> -
> - __overlay__ {
> - local: new-local-node {
> - new-property;
> - };
> - };
> - };
> -
> - fragment@6 {
> - target = <&test>;
> -
> - __overlay__ {
> - test-phandle = <&test>, <&local>;
> - };
> - };
> -
> - fragment@7 {
> - target = <&test>;
> -
> - __overlay__ {
> - test-several-phandle = <&local>, <&local>;
> - };
> - };
> -
> - fragment@8 {
> - target = <&test>;
> -
> - __overlay__ {
> - sub-test-node {
> - new-sub-test-property;
> - };
> - };
> - };
> -};
> +/include/ "overlay_overlay_dtc.dtsi"
Don't duplicate this, just replace it with the new style. This only
existed as essentially documentation for the libfdt overlay
application stuff. Since the new dtc won't support the old tag
format, there's no point having a test for it.
> diff --git a/tests/overlay_overlay_dtc.dtsi b/tests/overlay_overlay_dtc.dtsi
> new file mode 100644
> index 0000000..8ea8d5d
> --- /dev/null
> +++ b/tests/overlay_overlay_dtc.dtsi
> @@ -0,0 +1,83 @@
> +/*
> + * Copyright (c) 2016 NextThing Co
> + * Copyright (c) 2016 Free Electrons
> + * Copyright (c) 2016 Konsulko Inc.
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +/ {
> + /* Test that we can change an int by another */
> + fragment@0 {
> + target = <&test>;
> +
> + __overlay__ {
> + test-int-property = <43>;
> + };
> + };
> +
> + /* Test that we can replace a string by a longer one */
> + fragment@1 {
> + target = <&test>;
> +
> + __overlay__ {
> + test-str-property = "foobar";
> + };
> + };
> +
> + /* Test that we add a new property */
> + fragment@2 {
> + target = <&test>;
> +
> + __overlay__ {
> + test-str-property-2 = "foobar2";
> + };
> + };
> +
> + /* Test that we add a new node (by phandle) */
> + fragment@3 {
> + target = <&test>;
> +
> + __overlay__ {
> + new-node {
> + new-property;
> + };
> + };
> + };
> +
> + fragment@5 {
> + target = <&test>;
> +
> + __overlay__ {
> + local: new-local-node {
> + new-property;
> + };
> + };
> + };
> +
> + fragment@6 {
> + target = <&test>;
> +
> + __overlay__ {
> + test-phandle = <&test>, <&local>;
> + };
> + };
> +
> + fragment@7 {
> + target = <&test>;
> +
> + __overlay__ {
> + test-several-phandle = <&local>, <&local>;
> + };
> + };
> +
> + fragment@8 {
> + target = <&test>;
> +
> + __overlay__ {
> + sub-test-node {
> + new-sub-test-property;
> + };
> + };
> + };
> +};
> diff --git a/tests/overlay_overlay_new_dtc.dts b/tests/overlay_overlay_new_dtc.dts
> new file mode 100644
> index 0000000..14d3f54
> --- /dev/null
> +++ b/tests/overlay_overlay_new_dtc.dts
> @@ -0,0 +1,11 @@
> +/*
> + * Copyright (c) 2016 NextThing Co
> + * Copyright (c) 2016 Free Electrons
> + * Copyright (c) 2016 Konsulko Inc.
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +/dts-v1/ /plugin/;
> +
> +/include/ "overlay_overlay_dtc.dtsi"
> diff --git a/tests/overlay_overlay_simple.dts b/tests/overlay_overlay_simple.dts
> new file mode 100644
> index 0000000..8657e1e
> --- /dev/null
> +++ b/tests/overlay_overlay_simple.dts
> @@ -0,0 +1,12 @@
> +/*
> + * Copyright (c) 2016 Konsulko Inc.
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +/dts-v1/;
> +/plugin/;
> +
> +&test {
> + test-int-property = <43>;
> +};
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index e4139dd..74af0ff 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -181,6 +181,47 @@ overlay_tests () {
> run_dtc_test -@ -I dts -O dtb -o overlay_base_with_symbols.test.dtb overlay_base.dts
> run_dtc_test -@ -I dts -O dtb -o overlay_overlay_with_symbols.test.dtb overlay_overlay_dtc.dts
> run_test overlay overlay_base_with_symbols.test.dtb overlay_overlay_with_symbols.test.dtb
> +
> + # new /plugin/ format
> + run_dtc_test -@ -I dts -O dtb -o overlay_overlay_new_with_symbols.test.dtb overlay_overlay_new_dtc.dts
> + run_test check_path overlay_overlay_new_with_symbols.test.dtb exists "/__symbols__"
> + run_test check_path overlay_overlay_new_with_symbols.test.dtb exists "/__fixups__"
> + run_test check_path overlay_overlay_new_with_symbols.test.dtb exists "/__local_fixups__"
Looks like you're mixing tabs and spaces here. I don't really mind
which, but keep it consistent at least at the same indentation level.
> + # test new magic option
> + run_dtc_test -M@ -I dts -O dtb -o overlay_overlay_with_symbols_new_magic.test.dtb overlay_overlay_dtc.dts
> + run_test check_path overlay_overlay_with_symbols_new_magic.test.dtb exists "/__symbols__"
> + run_test check_path overlay_overlay_with_symbols_new_magic.test.dtb exists "/__fixups__"
> + run_test check_path overlay_overlay_with_symbols_new_magic.test.dtb exists "/__local_fixups__"
> +
> + # test plugin source to dtb and back
> + run_dtc_test -@ -I dtb -O dts -o overlay_overlay_dtc.test.dts overlay_overlay_with_symbols.test.dtb
> + run_dtc_test -@ -I dts -O dtb -o overlay_overlay_with_symbols.test.test.dtb overlay_overlay_dtc.test.dts
> + run_test dtbs_equal_ordered overlay_overlay_with_symbols.test.dtb overlay_overlay_with_symbols.test.test.dtb
> +
> + # test plugin source to dtb and back (with new magic)
> + run_dtc_test -@ -I dtb -O dts -o overlay_overlay_dtc_new_magic.test.dts overlay_overlay_with_symbols_new_magic.test.dtb
> + run_dtc_test -@ -I dts -O dtb -o overlay_overlay_with_symbols_new_magic.test.test.dtb overlay_overlay_dtc_new_magic.test.dts
> + run_test dtbs_equal_ordered overlay_overlay_with_symbols_new_magic.test.dtb overlay_overlay_with_symbols_new_magic.test.test.dtb
> +
> + # test plugin auto-generation without using -@
> + run_dtc_test -I dts -O dtb -o overlay_overlay_new_with_symbols_auto.test.dtb overlay_overlay_dtc.dts
> + run_test check_path overlay_overlay_new_with_symbols_auto.test.dtb exists "/__symbols__"
> + run_test check_path overlay_overlay_new_with_symbols_auto.test.dtb exists "/__fixups__"
> + run_test check_path overlay_overlay_new_with_symbols_auto.test.dtb exists "/__local_fixups__"
> +
> + # Test suppression of fixups
> + run_dtc_test -F -@ -I dts -O dtb -o overlay_base_with_symbols_no_fixups.test.dtb overlay_base.dts
> + run_test check_path overlay_base_with_symbols_no_fixups.test.dtb exists "/__symbols__"
> + run_test check_path overlay_base_with_symbols_no_fixups.test.dtb not-exists "/__fixups__"
> + run_test check_path overlay_base_with_symbols_no_fixups.test.dtb not-exists "/__local_fixups__"
> +
> + # Test generation of aliases insted of symbols
> + run_dtc_test -A -I dts -O dtb -o overlay_overlay_with_aliases.dtb overlay_overlay_dtc.dts
> + run_test check_path overlay_overlay_with_aliases.dtb exists "/aliases"
> + run_test check_path overlay_overlay_with_aliases.dtb exists "/__symbols__"
> + run_test check_path overlay_overlay_with_aliases.dtb exists "/__fixups__"
> + run_test check_path overlay_overlay_with_aliases.dtb exists "/__local_fixups__"
> fi
>
> # Bad fixup tests
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v11 5/7] overlay: Documentation for the overlay sugar syntax
From: David Gibson @ 2016-11-29 3:10 UTC (permalink / raw)
To: Pantelis Antoniou
Cc: Jon Loeliger, Grant Likely, Frank Rowand, Rob Herring, Jan Luebbe,
Sascha Hauer, Phil Elwell, Simon Glass, Maxime Ripard,
Thomas Petazzoni, Boris Brezillon, Antoine Tenart, Stephen Boyd,
Devicetree Compiler, devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1480349141-14145-6-git-send-email-pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1875 bytes --]
On Mon, Nov 28, 2016 at 06:05:39PM +0200, Pantelis Antoniou wrote:
> There exists a syntactic sugar version of overlays which
> make them simpler to write for the trivial case of a single target.
>
> Document it in the device tree object internals.
>
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
I'm with Frank that I think this, rather than being regarded mere
syntactic sugar, should be considered the primary way of describing
overlays.
Obviously we need to support the fully written out version as well.
> ---
> Documentation/dt-object-internal.txt | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/Documentation/dt-object-internal.txt b/Documentation/dt-object-internal.txt
> index 026d4ee..d5b841e 100644
> --- a/Documentation/dt-object-internal.txt
> +++ b/Documentation/dt-object-internal.txt
> @@ -300,3 +300,19 @@ local reference is being made. No matter how phandles are allocated from dtc
> the run time loader must apply an offset to each phandle in every dynamic
> DT object loaded. The __local_fixups__ node records the place of every
> local reference so that the loader can apply the offset.
> +
> +There is an alternative syntax to the expanded form for overlays with phandle
> +targets which makes the format similar to the one using in .dtsi include files.
> +
> +So for the &ocp target example above one can simply write:
> +
> +/dts-v1/ /plugin/;
> +&ocp {
> + /* bar peripheral */
> + bar {
> + compatible = "corp,bar";
> + ... /* various properties and child nodes */
> + }
> +};
> +
> +The resulting dtb object is identical.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v2 2/4] usb: dwc2: Add binding for AHB burst
From: John Youn @ 2016-11-29 3:32 UTC (permalink / raw)
To: Christian Lamparter, John Youn
Cc: Rob Herring, Stefan Wahren, Felipe Balbi,
linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Mark Rutland,
linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
Michael Ellerman, Paul Mackerras, Benjamin Herrenschmidt
In-Reply-To: <1495076.fZ1uLW9fli@debian64>
On 11/22/2016 12:51 PM, Christian Lamparter wrote:
> On Monday, November 21, 2016 7:32:30 PM CET John Youn wrote:
>> On 11/21/2016 1:10 PM, Christian Lamparter wrote:
>>> On Monday, November 21, 2016 12:16:31 PM CET John Youn wrote:
>>>> On 11/18/2016 12:18 PM, Christian Lamparter wrote:
>>>>> On Friday, November 18, 2016 8:16:08 AM CET Rob Herring wrote:
>>>>>> Also, perhaps you should allow that the compatible string can define the
>>>>>> default.
>>>>>>
>>>>> I hoped you would say that :).
>>>>>
>>>>> I've attached a patch (on top of John Youn changes) [...]
>>>>> ---
>>>>> Subject: [PATCH] usb: dwc2: add a default ahb-burst setting for amcc,dwc-otg
>>>>> [...]
>>>>> @@ -1097,6 +1097,22 @@ static const char *const ahb_bursts[] = {
>>>>> +/* [...] */
>>>>> +static const struct of_device_id dwc2_compat_ahb_bursts[] = {
>>>>> + {
>>>>> + .compatible = "amcc,dwc-otg",
>>>>> + .data = (void *) GAHBCFG_HBSTLEN_INCR16,
>>>>> + },
>>>>> +};
>> [...]
>>>>>> @@ -1107,6 +1123,12 @@ static int dwc2_get_property_ahb_burst(struct dwc2_hsotg *hsotg)
>>>>> ret = device_property_read_string(hsotg->dev, "snps,ahb-burst", &str);
>>>>> if (ret < 0) {
>>>>> + const struct of_device_id *match;
>>>>> +
>>>>> + match = of_match_node(dwc2_compat_ahb_bursts, node);
>>>>> + if (match)
>>>>> + ret = (int)match->data;
>>>>> +
>> [...]
>>>> I'd prefer if you use the binding which requires no extra code in
>>>> dwc2.
>>> I'm fine with either option. However it think that this would require
>>> that either Mark or Rob would allow an exception to the "keep existing
>>> dts the way they are) and ack the following change to the canyonlands.dts.
>>
>> I don't know about that. Under what circumstance can the dts change?
> As far as I know, the justification for not changing the DTS is that a
> compiled DTB might be stored in an read-only ROM on a board. So it would
> be impossible to update it. Hence, the driver have work with the existing
> (and sometimes buggy or incomplete) information to stay compatible.
>
> (Note: Thankfully, the canyonlands dtb is stored in flash, it's possible
> to update it. But it is an extra step that's not done automatically
> with make install).
>
>> The canyonlands dts was binding to an external vendor driver. So it
>> wasn't documented nor expected to work with dwc2 until your recent
>> patch adding the compatible string.
>
> Oh, no that's not what happend. Let me explain why there was no "external
> vendor driver": AMCC/APM were planing to upstream their hole platform. And
> in fact, the devs tried very hard to include their driver back in 2011 [0].
> But this driver was denied inclusion back then due to:
>
> "[...]
> I would also like to point out that the same Synopsys USB controller
> is used in a number of other SoCs (especially ARM chips), and
> supported by other drivers, some of these even in mainline.
>
> See http://thread.gmane.org/gmane.linux.usb.general/61714/focus=62139
> for a related thread.
>
> Instead of trying to add a completely new driver to mainline (and one
> which has been repeatedly been rejected), I vote for focusing on the
> existing driver code that is already in mainline, and testing and
> improving this so we can use a single implementation of this driver
> code for all SoCs that use the same IP block." [1]
>
> Of course: The listed link goes the "USB Host driver for i.MX28" driver.
> And this is an ehci-hcd like driver... Which is as you are well aware not
> that similar to the dwc2 OTG. And as far as I can tell: AMCC abandoned
> the patch series right there.
>
> Note: AMCC did however succeed in pushing your employer's Synopsys
> DesignWare SATA and DMA drivers to the kernel back then. And I'm happy
> to report that both drivers are still around and working fine for the 460EX
> (sata_dwc_460ex.c[2] and the DW AHB DMA [3]). (The drivers also work for
> different platforms than the original PPC. I know that because I helped
> Andy Shevchenko with testing and pushing some fixes to it when he was
> adding support for the Intel Quark SoC, which uses the DWC SATA and DMA).
Ok thanks for clearing that up. I understand.
For now we can just set the property to "INCR16" based on the
compatible string. Perhaps in the future do this from a glue-layer
driver which binds to all compatible strings other than "snps,dwc2".
I won't be able to do anything with this until next week though.
Regards,
John
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFC PATCH 1/2] net: macb: Add MDIO driver for accessing multiple PHY devices
From: Harini Katakam @ 2016-11-29 4:11 UTC (permalink / raw)
To: Andrew Lunn
Cc: Harini Katakam, Nicolas Ferre, davem, Rob Herring, Pawel Moll,
Mark Rutland, ijc+devicetree@hellion.org.uk, Kumar Gala,
Boris Brezillon, alexandre.belloni, netdev,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
michals@xilinx.com, Punnaiah Choudary Kalluri
In-Reply-To: <20161128163356.GJ17704@lunn.ch>
Hi Andrew,
On Mon, Nov 28, 2016 at 10:03 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> On Mon, Nov 28, 2016 at 03:19:14PM +0530, Harini Katakam wrote:
>> This patch is to add support for the hardware with multiple ethernet
>> MAC controllers and a single MDIO bus connected to multiple PHY devices.
>> MDIO lines are connected to any one of the ethernet MAC controllers and
>> all the PHY devices will be accessed using the PHY maintenance interface
>> in that MAC controller. This handling along with PHY functionality is
>> moved to macb_mdio.c
>>
>> Signed-off-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>
>> Signed-off-by: Harini Katakam <harinik@xilinx.com>
>> ---
>> drivers/net/ethernet/cadence/Makefile | 2 +-
>> drivers/net/ethernet/cadence/macb.c | 169 +++-----------------
>> drivers/net/ethernet/cadence/macb.h | 2 +
>> drivers/net/ethernet/cadence/macb_mdio.c | 266 +++++++++++++++++++++++++++++++
>> 4 files changed, 294 insertions(+), 145 deletions(-)
>> create mode 100644 drivers/net/ethernet/cadence/macb_mdio.c
>>
<snip>
>> + bus->irq = devm_kzalloc(&pdev->dev, sizeof(int) * PHY_MAX_ADDR,
>> + GFP_KERNEL);
>
> This looks wrong, or at least old. It used to be a pointer to an array,
> but it is now an actual array.
Sorry, this was a mistake.
I changed this after rebase, will update in next version.
>
>> +static const struct of_device_id macb_mdio_dt_ids[] = {
>> + { .compatible = "cdns,macb-mdio" },
>> +
>> +};
>
>
> I've not looked hard enough to know, but can you keep backwards
> compatibility? Won't old device tree's assume the mdio bus is always
> present? Now you need an explicit node otherwise there will not be an
> mdio bus?
Yes, an explicit MDIO bus is required. But I'm not sure
how to maintain backward compatibility (without using this separate
macb_mdio) and have different MACs use the same MDIO bus
with separate PHYs.
Regards,
Harini
^ permalink raw reply
* Re: [PATCH v11 5/7] overlay: Documentation for the overlay sugar syntax
From: Frank Rowand @ 2016-11-29 4:36 UTC (permalink / raw)
To: David Gibson, Pantelis Antoniou
Cc: Jon Loeliger, Grant Likely, Rob Herring, Jan Luebbe, Sascha Hauer,
Phil Elwell, Simon Glass, Maxime Ripard, Thomas Petazzoni,
Boris Brezillon, Antoine Tenart, Stephen Boyd,
Devicetree Compiler, devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20161129031054.GI13307-K0bRW+63XPQe6aEkudXLsA@public.gmane.org>
On 11/28/16 19:10, David Gibson wrote:
> On Mon, Nov 28, 2016 at 06:05:39PM +0200, Pantelis Antoniou wrote:
>> There exists a syntactic sugar version of overlays which
>> make them simpler to write for the trivial case of a single target.
It also works for multiple targets. (See the example I provided in
my comment to v10.)
>>
>> Document it in the device tree object internals.
>>
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>
> I'm with Frank that I think this, rather than being regarded mere
> syntactic sugar, should be considered the primary way of describing
> overlays.
>
> Obviously we need to support the fully written out version as well.
If we need to support the fully written out version, can we make that
a discouraged, non-preferred method? Maybe require an option to
enable compiling this style of dts?
I can imagine some reasons to support the fully written out version,
but can we document what those reasons are?
-Frank
>
>> ---
>> Documentation/dt-object-internal.txt | 16 ++++++++++++++++
>> 1 file changed, 16 insertions(+)
>>
>> diff --git a/Documentation/dt-object-internal.txt b/Documentation/dt-object-internal.txt
>> index 026d4ee..d5b841e 100644
>> --- a/Documentation/dt-object-internal.txt
>> +++ b/Documentation/dt-object-internal.txt
>> @@ -300,3 +300,19 @@ local reference is being made. No matter how phandles are allocated from dtc
>> the run time loader must apply an offset to each phandle in every dynamic
>> DT object loaded. The __local_fixups__ node records the place of every
>> local reference so that the loader can apply the offset.
>> +
>> +There is an alternative syntax to the expanded form for overlays with phandle
>> +targets which makes the format similar to the one using in .dtsi include files.
>> +
>> +So for the &ocp target example above one can simply write:
>> +
>> +/dts-v1/ /plugin/;
>> +&ocp {
>> + /* bar peripheral */
>> + bar {
>> + compatible = "corp,bar";
>> + ... /* various properties and child nodes */
>> + }
>> +};
>> +
>> +The resulting dtb object is identical.
>
^ permalink raw reply
* Re: [PATCH v11 5/7] overlay: Documentation for the overlay sugar syntax
From: David Gibson @ 2016-11-29 5:10 UTC (permalink / raw)
To: Frank Rowand
Cc: Pantelis Antoniou, Jon Loeliger, Grant Likely, Rob Herring,
Jan Luebbe, Sascha Hauer, Phil Elwell, Simon Glass, Maxime Ripard,
Thomas Petazzoni, Boris Brezillon, Antoine Tenart, Stephen Boyd,
Devicetree Compiler, devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <583D05B7.4040109-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1816 bytes --]
On Mon, Nov 28, 2016 at 08:36:07PM -0800, Frank Rowand wrote:
> On 11/28/16 19:10, David Gibson wrote:
> > On Mon, Nov 28, 2016 at 06:05:39PM +0200, Pantelis Antoniou wrote:
> >> There exists a syntactic sugar version of overlays which
> >> make them simpler to write for the trivial case of a single target.
>
> It also works for multiple targets. (See the example I provided in
> my comment to v10.)
>
>
> >>
> >> Document it in the device tree object internals.
> >>
> >> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> >
> > I'm with Frank that I think this, rather than being regarded mere
> > syntactic sugar, should be considered the primary way of describing
> > overlays.
> >
> > Obviously we need to support the fully written out version as well.
>
> If we need to support the fully written out version, can we make that
> a discouraged, non-preferred method? Maybe require an option to
> enable compiling this style of dts?
Yeah. To avoid further proliferation of options, I'm thinking a
single "backwards compat" option which would:
- Use the dtb magic instead of dtb magic
- Disable checks which would reject explicit creation of
__overlay__ / __symbols__ / __fixups__ nodes
- Anything other special behaviour we need
> I can imagine some reasons to support the fully written out version,
> but can we document what those reasons are?
I believe the main one is the dts files in this format out in the
field. Mind you, I guess we're already requiring them to tweak how
they declare the /plugin/ option.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/4] dt-bindings: phy: Add support for QUSB2 phy
From: Vivek Gautam @ 2016-11-29 5:20 UTC (permalink / raw)
To: Rob Herring
Cc: kishon, Mark Rutland, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, Srinivas Kandagatla, Stephen Boyd,
linux-arm-msm
In-Reply-To: <20161128141944.4sfptmymh5kcpcwm@rob-hp-laptop>
Hi Rob,
On Mon, Nov 28, 2016 at 7:49 PM, Rob Herring <robh@kernel.org> wrote:
Thanks for reviewing the patch.
> On Tue, Nov 22, 2016 at 05:32:40PM +0530, Vivek Gautam wrote:
>> Qualcomm chipsets have QUSB2 phy controller that provides
>> HighSpeed functionality for DWC3 controller.
>> Adding dt binding information for the same.
>>
>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
>> ---
>>
>> Changes since v1:
>> - New patch, forked out of the original driver patch:
>> "phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom chips"
>> - Updated dt bindings to remove 'hstx-trim-bit-offset' and
>> 'hstx-trim-bit-len' bindings.
>>
>> .../devicetree/bindings/phy/qcom-qusb2-phy.txt | 55 ++++++++++++++++++++++
>> 1 file changed, 55 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
>>
>> diff --git a/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
>> new file mode 100644
>> index 0000000..38c8b30
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
>> @@ -0,0 +1,55 @@
>> +Qualcomm QUSB2 phy controller
>> +=============================
>> +
>> +QUSB2 controller supports LS/FS/HS usb connectivity on Qualcomm chipsets.
>> +
>> +Required properties:
>> + - compatible: compatible list, contains "qcom,msm8996-qusb2-phy".
>> + - reg: offset and length of the PHY register set.
>> + - #phy-cells: must be 0.
>> +
>> + - clocks: a list of phandles and clock-specifier pairs,
>> + one for each entry in clock-names.
>> + - clock-names: must be "cfg_ahb" for phy config clock,
>> + "ref_clk" for 19.2 MHz ref clk,
>> + "ref_clk_src" reference clock source.
>> + "iface" for phy interface clock (Optional).
>> +
>> + - vdd-phy-supply: Phandle to a regulator supply to PHY core block.
>> + - vdda-pll-supply: Phandle to 1.8V regulator supply to PHY refclk pll block.
>> + - vdda-phy-dpdm: Phandle to 3.1V regulator supply to Dp/Dm port signals.
>
> Needs '-supply'
sure, will add.
>
>> +
>> + - resets: a list of phandles and reset controller specifier pairs,
>> + one for each entry in reset-names.
>> + - reset-names: must be "phy" for reset of phy block.
>> +
>> +Optional properties:
>> + - nvmem-cells: a list of phandles to nvmem cells that contain fused
>> + tuning parameters for qusb2 phy, one for each entry
>> + in nvmem-cell-names.
>> + - nvmem-cell-names: must be "tune2_hstx_trim_efuse" for cell containing
>> + HS Tx trim value.
>> +
>> + - qcom,tcsr-syscon: Phandle to TCSR syscon register region.
>> +
>> +Example:
>> + hsphy: qusb2phy@7411000 {
>
> usb-phy@...
Or may be just 'phy' for the node name, and then label could be 'usb_hs_phy'?
[...]
Thanks
Vivek
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox