* Re: [RFC PATCH 14/15] regulator: pwm: implement ->enable(), ->disable() and ->is_enabled methods
From: Boris Brezillon @ 2015-07-01 12:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <3530235.H11CYEfk28@diego>
On Wed, 01 Jul 2015 14:08:18 +0200
Heiko Stübner <heiko@sntech.de> wrote:
> Am Mittwoch, 1. Juli 2015, 14:05:31 schrieb Boris Brezillon:
> > Hi Heiko,
> >
> > On Wed, 01 Jul 2015 13:58:09 +0200
> >
> > Heiko Stübner <heiko@sntech.de> wrote:
> > > Am Mittwoch, 1. Juli 2015, 10:22:00 schrieb Boris Brezillon:
> > > > Implement the ->enable(), ->disable() and ->is_enabled methods and
> > > > remove
> > > > the PWM call in ->set_voltage_sel().
> > > > This is particularly important for critical regulators tagged as
> > > > always-on,
> > > > because not claiming the PWM (and its dependencies) might lead to
> > > > unpredictable behavior (like a system hang because the PWM clk is only
> > > > claimed when the PWM device is enabled).
> > > >
> > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > > ---
> > > >
> > > > drivers/regulator/pwm-regulator.c | 32 ++++++++++++++++++++++++++------
> > > > 1 file changed, 26 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/regulator/pwm-regulator.c
> > > > b/drivers/regulator/pwm-regulator.c index 12b4d9d..8159518 100644
> > > > --- a/drivers/regulator/pwm-regulator.c
> > > > +++ b/drivers/regulator/pwm-regulator.c
> > > > @@ -59,12 +59,6 @@ static int pwm_regulator_set_voltage_sel(struct
> > > > regulator_dev *rdev,
> > > >
> > > > drvdata->state = selector;
> > > >
> > > > - ret = pwm_enable(drvdata->pwm);
> > > > - if (ret) {
> > > > - dev_err(&rdev->dev, "Failed to enable PWM\n");
> > > > - return ret;
> > > > - }
> > > > -
> > > >
> > > > return 0;
> > > >
> > > > }
> > > >
> > > > @@ -79,11 +73,37 @@ static int pwm_regulator_list_voltage(struct
> > > > regulator_dev *rdev, return drvdata->duty_cycle_table[selector].uV;
> > > >
> > > > }
> > > >
> > > > +static int pwm_regulator_enable(struct regulator_dev *dev)
> > > > +{
> > > > + struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
> > > > +
> > > > + return pwm_enable(drvdata->pwm);
> > > > +}
> > > > +
> > > > +static int pwm_regulator_disable(struct regulator_dev *dev)
> > > > +{
> > > > + struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
> > > > +
> > > > + pwm_disable(drvdata->pwm);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int pwm_regulator_is_enabled(struct regulator_dev *dev)
> > > > +{
> > > > + struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
> > > > +
> > > > + return pwm_is_enabled(drvdata->pwm);
> > > > +}
> > >
> > > nit: indentation is wrong in pwm_regulator_is_enabled (spaces instead of
> > > tabs)
> > Yep, I noticed checkpatch warnings/errors before sending the patch, but
> > since this is just an RFC I decided to fix them for the next version ;-)
>
> ok, so I'll just skip over any more style issues for now. Making my way
> through your series and trying it on my veyron right now :-)
Also note that I haven't tested the series on a real board (just compile
tested) because I don't have the board with me right now, but I wanted
to post the RFC early so that we can discuss the concepts.
Anyway, any feedback on the implementation (including bug reports) is
welcome.
This is the version I actually tested on the veyron board:
https://github.com/bbrezillon/linux-rk/tree/rk-3.14
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH v2 11/12] ASoC: tegra: register dependency parser for firmware nodes
From: Mark Brown @ 2015-07-01 17:38 UTC (permalink / raw)
To: Tomeu Vizoso
Cc: linux-kernel, linux-acpi, dri-devel, linux-fbdev, linux-gpio,
devicetree, linux-pwm, Rafael J. Wysocki, alsa-devel,
Jaroslav Kysela, Thierry Reding, Takashi Iwai, Stephen Warren,
Liam Girdwood, linux-tegra, Alexandre Courbot
In-Reply-To: <1435743667-11987-12-git-send-email-tomeu.vizoso@collabora.com>
[-- Attachment #1: Type: text/plain, Size: 552 bytes --]
On Wed, Jul 01, 2015 at 11:41:06AM +0200, Tomeu Vizoso wrote:
> +static void tegra_max98090_get_dependencies(struct fwnode_handle *fwnode,
> + struct list_head *deps)
> +{
> + add_dependency(fwnode, "nvidia,i2s-controller", deps);
> + add_dependency(fwnode, "nvidia,audio-codec", deps);
> +}
Why is this all being open coded in an individual driver (we already
know about and manage all these dependencies in the core...)? If we're
going to do this I'd expect the interface for specifying DT nodes to the
core to be changed to support this.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 12/15] pwm: rockchip: add initial state retrieval
From: Heiko Stübner @ 2015-07-01 21:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1435738921-25027-13-git-send-email-boris.brezillon@free-electrons.com>
Hi Boris,
Am Mittwoch, 1. Juli 2015, 10:21:58 schrieb Boris Brezillon:
> Implement the ->init_state() function to expose initial state.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
[...]
> @@ -98,6 +110,36 @@ static void rockchip_pwm_set_enable_v2(struct pwm_chip
> *chip, writel_relaxed(val, pc->base + pc->data->regs.ctrl);
> }
>
> +static void rockchip_pwm_init_state(struct pwm_chip *chip,
> + struct pwm_device *pwm)
> +{
> + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
> + unsigned long clk_rate;
> + u64 tmp;
> + int ret;
> +
> + ret = clk_enable(pc->clk);
> + if (ret)
> + return;
> +
> + clk_rate = clk_get_rate(pc->clk);
> +
> + tmp = readl(pc->base + pc->data->regs.period);
> + tmp *= pc->data->prescaler * NSEC_PER_SEC;
> + tmp = do_div(tmp, clk_rate);
I guess you want to have the division result here and not the remainder, so
- tmp = do_div(tmp, clk_rate);
+ do_div(tmp, clk_rate);
> + pwm->state.period = tmp;
> +
> + tmp = readl(pc->base + pc->data->regs.duty);
> + tmp *= pc->data->prescaler * NSEC_PER_SEC;
> + tmp = do_div(tmp, clk_rate);
ditto
> + pwm->state.duty_cycle = tmp;
> +
> + pc->data->init(chip, chip->pwms);
> +
> + if (!pwm_is_enabled(pwm))
> + clk_disable(pc->clk);
> +}
> +
> static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device
> *pwm, int duty_ns, int period_ns)
> {
> @@ -171,6 +213,7 @@ static void rockchip_pwm_disable(struct pwm_chip *chip,
> struct pwm_device *pwm) }
>
> static const struct pwm_ops rockchip_pwm_ops_v1 = {
> + .init_state = rockchip_pwm_init_state,
> .config = rockchip_pwm_config,
> .enable = rockchip_pwm_enable,
> .disable = rockchip_pwm_disable,
> @@ -178,6 +221,7 @@ static const struct pwm_ops rockchip_pwm_ops_v1 = {
> };
>
> static const struct pwm_ops rockchip_pwm_ops_v2 = {
> + .init_state = rockchip_pwm_init_state,
> .config = rockchip_pwm_config,
> .set_polarity = rockchip_pwm_set_polarity,
> .enable = rockchip_pwm_enable,
> @@ -195,6 +239,7 @@ static const struct rockchip_pwm_data pwm_data_v1 = {
> .prescaler = 2,
> .ops = &rockchip_pwm_ops_v1,
> .set_enable = rockchip_pwm_set_enable_v1,
> + .init = rockchip_pwm_init_v1,
> };
>
> static const struct rockchip_pwm_data pwm_data_v2 = {
> @@ -207,6 +252,7 @@ static const struct rockchip_pwm_data pwm_data_v2 = {
> .prescaler = 1,
> .ops = &rockchip_pwm_ops_v2,
> .set_enable = rockchip_pwm_set_enable_v2,
> + .init = rockchip_pwm_init_v2,
you're referencing the v2 init here, but only add it in the next patch?
[pwm: rockchip: add support for atomic update]
> };
>
> static const struct rockchip_pwm_data pwm_data_vop = {
> @@ -219,6 +265,7 @@ static const struct rockchip_pwm_data pwm_data_vop = {
> .prescaler = 1,
> .ops = &rockchip_pwm_ops_v2,
> .set_enable = rockchip_pwm_set_enable_v2,
> + .init = rockchip_pwm_init_v2,
ditto
Heiko
^ permalink raw reply
* Re: [RFC PATCH 13/15] pwm: rockchip: add support for atomic update
From: Heiko Stübner @ 2015-07-01 21:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1435738921-25027-14-git-send-email-boris.brezillon@free-electrons.com>
Hi Boris,
Am Mittwoch, 1. Juli 2015, 10:21:59 schrieb Boris Brezillon:
> Implement the ->apply() function to add support for atomic update.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> @@ -110,6 +113,26 @@ static void rockchip_pwm_set_enable_v2(struct pwm_chip
> *chip, writel_relaxed(val, pc->base + pc->data->regs.ctrl);
> }
>
> +static void rockchip_pwm_init_v2(struct pwm_chip *chip, struct pwm_device
> *pwm) +{
> + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
> + u32 enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | PWM_ENABLE |
> + PWM_CONTINUOUS;
> + u32 val;
> +
> + val = readl(pc->base + pc->data->regs.ctrl);
> +
> + if ((val & enable_conf) != enable_conf)
> + return;
> +
> + pwm->state.enabled = true;
> +
> + enable_conf = PWM_DUTY_NEGATIVE | PWM_INACTIVE_POSITIVE;
> +
> + if ((val & enable_conf) = enable_conf)
> + pwm->state.polarity = PWM_POLARITY_INVERSED;
the inactive setting does not affect the polarity of the running pwm, only what
to do when it gets turned off. Also PWM_DUTY_NEGATIVE is the "0" value for the
bit so also is bad to compare against (and results in wrong readings). So I
would suggest changing this like
- enable_conf = PWM_DUTY_NEGATIVE | PWM_INACTIVE_POSITIVE;
+ enable_conf = PWM_DUTY_POSITIVE;
- if ((val & enable_conf) = enable_conf)
+ if ((val & enable_conf) != enable_conf)
> +}
> +
> static void rockchip_pwm_init_state(struct pwm_chip *chip,
> struct pwm_device *pwm)
> {
Heiko
^ permalink raw reply
* [RFC PATCH 16/15] pwm: add informations about polarity, duty cycle and period to debugfs
From: Heiko Stübner @ 2015-07-01 21:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1435738921-25027-1-git-send-email-boris.brezillon@free-electrons.com>
The pwm-states make it possible to also output the polarity, duty cycle
and period information in the debugfs pwm summary-outout.
This makes it easier to gather overview information about pwms without
needing to walk through the sysfs attributes of every pwm.
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
might be nice to have too ;-)
drivers/pwm/core.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 6dafd8e..79037a2 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -951,9 +951,18 @@ static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
if (test_bit(PWMF_REQUESTED, &pwm->flags))
seq_puts(s, " requested");
- if (pwm_is_enabled(pwm))
+ if (pwm_is_enabled(pwm)) {
seq_puts(s, " enabled");
+ seq_printf(s, " period:%uns",
+ pwm_get_period(pwm));
+ seq_printf(s, " duty:%uns",
+ pwm_get_duty_cycle(pwm));
+ seq_printf(s, " polarity:%s",
+ pwm_get_polarity(pwm) ? "inverse"
+ : "normal");
+ }
+
seq_puts(s, "\n");
}
}
--
2.1.4
^ permalink raw reply related
* Re: [RFC PATCH 00/15] pwm: add support for atomic update
From: Heiko Stübner @ 2015-07-01 21:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1435738921-25027-1-git-send-email-boris.brezillon@free-electrons.com>
Hi Boris,
Am Mittwoch, 1. Juli 2015, 10:21:46 schrieb Boris Brezillon:
> Hello Thierry,
>
> This series adds support for atomic PWM update, or ITO, the capability
> to update all the parameters of a PWM device (enabled/disabled, period,
> duty and polarity) in one go.
>
> This implementation is still experimental, and I may have missed some key
> aspect, so any feedback are welcome.
>
> Also note that I haven't protected the state update with any locking.
> That's because the existing config does not protect against concurrent
> access to a requested PWM device (see the pwm_config implementation).
> I guess the PWM framework assume the user will implement the proper locking
> scheme if it has to concurrently access the device.
>
> The 5 first patches prepare the addition of the pwm_state concept, which
> will be used to allow atomic updates.
> The following patches introduce the pwm_state struct, initial state
> retrieval and atomic update concepts.
>
> Patches 12 and 13 are showing how one can implement the initial state
> retrieval and atomic update features in a PWM driver (in this specific
> case I implemented it in the rockchip driver).
>
> The last 2 patches are making use of those changes to improve the
> pwm-regulator driver (initializing the regulator state based on the
> initial PWM state).
at first I got very strange readings (very wrong values and wrong polarity),
which resulted from the issues I pointed out in the replies to individual
patches. After fixing these, the pwm read-back now returns exactly the expected
values :-) .
And with the original voltage table from the Chromeos-devicetrees, the pwm-
regulator also returns the expected 1.2V that coreboot initially set.
Heiko
^ permalink raw reply
* Re: [PATCH v2 01/12] device: property: delay device-driver matches
From: Rafael J. Wysocki @ 2015-07-01 23:29 UTC (permalink / raw)
To: Tomeu Vizoso
Cc: linux-kernel, Mark Brown, linux-acpi, dri-devel, linux-fbdev,
linux-gpio, devicetree, linux-pwm, alsa-devel, Greg Kroah-Hartman
In-Reply-To: <1435743667-11987-2-git-send-email-tomeu.vizoso@collabora.com>
On Wednesday, July 01, 2015 11:40:56 AM Tomeu Vizoso wrote:
> Delay matches of platform devices until late_initcall, when we are sure
> that all built-in drivers have been registered already. This is needed
> to prevent deferred probes because of some dependencies' drivers not
> having registered yet.
>
> This reduces the total amount of work that the kernel does during boot
> because it won't try to match devices to drivers when built-in drivers
> are still registering but also reduces some parallelism, so total boot
> time might slightly increase or decrease depending on the platform and
> kernel configuration.
>
> This change will make make possible to prevent any deferred probes once
> devices are probed in dependency order.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
>
> Changes in v2:
> - Instead of delaying all probes until late_initcall, only delay matches
> of platform devices that have a firmware node attached.
>
> drivers/base/property.c | 29 +++++++++++++++++++++++++++++
> 1 file changed, 29 insertions(+)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 8528eb9..8ead1ba 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -16,8 +16,11 @@
> #include <linux/of.h>
> #include <linux/of_address.h>
> #include <linux/of_device.h>
> +#include <linux/platform_device.h>
> #include <linux/property.h>
>
> +static bool fwnode_match_enable = false;
> +
> /**
> * device_add_property_set - Add a collection of properties to a device object.
> * @dev: Device to add properties to.
> @@ -604,6 +607,15 @@ EXPORT_SYMBOL_GPL(fwnode_is_compatible);
> bool fwnode_driver_match_device(struct device *dev,
> const struct device_driver *drv)
> {
> + /*
> + * Delay matches of platform devices until late_initcall, when we are
> + * sure that all built-in drivers have been registered already. This
> + * is needed to prevent deferred probes because of some drivers
> + * not having registered yet.
> + */
> + if(dev->bus = &platform_bus_type && !fwnode_match_enable)
> + return false;
I'm not particularly enthusiastic about referring to specific bus types in
generic code like that.
What about having a special version of fwnode_driver_match_device() specifically
for the platform bus type that will do the check?
> +
> if (is_of_node(dev->fwnode))
> return of_driver_match_device(dev, drv);
> else if (is_acpi_node(dev->fwnode))
> @@ -612,3 +624,20 @@ bool fwnode_driver_match_device(struct device *dev,
> return false;
> }
> EXPORT_SYMBOL_GPL(fwnode_driver_match_device);
> +
> +static int __device_attach(struct device *dev, void *data)
> +{
> + device_initial_probe(dev);
> +
> + return 0;
> +}
> +
> +static int fwnode_match_initcall(void)
> +{
> + fwnode_match_enable = true;
> +
> + bus_for_each_dev(&platform_bus_type, NULL, NULL, __device_attach);
> +
> + return 0;
> +}
> +late_initcall(fwnode_match_initcall);
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: [PATCH v2 02/12] device: property: find dependencies of a firmware node
From: Rafael J. Wysocki @ 2015-07-01 23:36 UTC (permalink / raw)
To: Tomeu Vizoso
Cc: linux-kernel, Mark Brown, linux-acpi, dri-devel, linux-fbdev,
linux-gpio, devicetree, linux-pwm, alsa-devel, Greg Kroah-Hartman
In-Reply-To: <1435743667-11987-3-git-send-email-tomeu.vizoso@collabora.com>
On Wednesday, July 01, 2015 11:40:57 AM Tomeu Vizoso wrote:
> Adds API that allows callers to find out what other firmware nodes a
> node depends on.
>
> Implementors of bindings documentation can register callbacks that
> return the dependencies of a node.
>
> Dependency information can be used to change the order in which devices
> are probed, or to print a warning when a device node is going to be
> probed without all its dependencies fulfilled.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
I'd like to see a description of the new API in English in the changelog.
> ---
>
> Changes in v2:
> - Allow bindings implementations register a function instead of using
> class callbacks, as not only subsystems implement firmware bindings.
>
> drivers/base/property.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/fwnode.h | 5 +++
> include/linux/property.h | 12 +++++++
> 3 files changed, 108 insertions(+)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 8ead1ba..9d38ede 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -19,7 +19,13 @@
> #include <linux/platform_device.h>
> #include <linux/property.h>
>
Please add a comment describing this structure. In particular, what it is
supposed to be used for and how it is supposed to be used.
> +struct dependency_parser {
> + struct list_head parser;
I'd rather call this "list_node" or something like that.
> + void (*func)(struct fwnode_handle *fwnode, struct list_head *deps);
> +};
> +
> static bool fwnode_match_enable = false;
> +static LIST_HEAD(dependency_parsers);
>
> /**
> * device_add_property_set - Add a collection of properties to a device object.
> @@ -553,6 +559,27 @@ bool device_dma_is_coherent(struct device *dev)
> EXPORT_SYMBOL_GPL(device_dma_is_coherent);
>
> /**
> + * fwnode_add_dependency - add firmware node to the passed dependency list
> + * @fwnode: Firmware node to add to dependency list
> + * @list: Dependency list to add the fwnode to
> + */
> +void fwnode_add_dependency(struct fwnode_handle *fwnode,
> + struct list_head *list)
> +{
> + struct fwnode_dependency *dep;
> +
> + dep = kzalloc(sizeof(*dep), GFP_KERNEL);
> + if (!dep)
> + return;
> +
> + INIT_LIST_HEAD(&dep->dependency);
> + dep->fwnode = fwnode;
> +
> + list_add_tail(&dep->dependency, list);
> +}
> +EXPORT_SYMBOL_GPL(fwnode_add_dependency);
> +
> +/**
> * fwnode_get_parent - return the parent node of a device node
> * @fwnode: Device node to find the parent node of
> */
> @@ -600,6 +627,70 @@ bool fwnode_is_compatible(struct fwnode_handle *fwnode, const char *compatible)
> EXPORT_SYMBOL_GPL(fwnode_is_compatible);
>
> /**
> + * fwnode_add_dependency_parser - register dependency parser
> + * @func: Function that will be called to find out dependencies of a node
> + *
> + * Registers a callback that will be called when collecting the dependencies
> + * of a firmware node. The callback should inspect the properties of the node
> + * and call fwnode_add_dependency() for each dependency it recognizes, from
> + * the bindings documentation.
> + */
> +void fwnode_add_dependency_parser(
> + void (*func)(struct fwnode_handle *fwnode, struct list_head *deps))
> +{
> + struct dependency_parser *parser;
> +
> + parser = kzalloc(sizeof(*parser), GFP_KERNEL);
> + if (!parser)
> + return;
> +
> + INIT_LIST_HEAD(&parser->parser);
> + parser->func = func;
> +
> + list_add_tail(&parser->parser, &dependency_parsers);
We're modifying a global list here. Any locking needed? RCU? Whatever?
> +}
> +EXPORT_SYMBOL_GPL(fwnode_add_dependency_parser);
> +
> +/**
> + * fwnode_remove_dependency_parser - unregister dependency parser
> + * @func: Function that was to be called to find out dependencies of a node
> + */
> +void fwnode_remove_dependency_parser(
> + void (*func)(struct fwnode_handle *fwnode, struct list_head *deps))
> +{
> + struct dependency_parser *parser, *tmp;
> +
> + list_for_each_entry_safe(parser, tmp, &dependency_parsers, parser) {
> + if (parser->func = func) {
> + list_del(&parser->parser);
> + kfree(parser);
> + return;
> + }
> + }
> +}
> +EXPORT_SYMBOL_GPL(fwnode_remove_dependency_parser);
> +
> +/**
> + * fwnode_get_dependencies - find out what dependencies a firmware node has
> + * @fwnode: firmware node to find its dependencies
> + * @deps: list of struct fwnode_dependency in which dependencies will be placed
> + */
> +void fwnode_get_dependencies(struct fwnode_handle *fwnode,
> + struct list_head *deps)
> +{
> + struct dependency_parser *parser;
> + struct fwnode_handle *child;
> +
> + list_for_each_entry(parser, &dependency_parsers, parser)
> + parser->func(fwnode, deps);
> +
> + /* Some device nodes will have dependencies in non-device sub-nodes */
> + fwnode_for_each_child_node(fwnode, child)
> + if (!fwnode_property_present(child, "compatible"))
This is a blatant OF-ism. We need to think about a generic way to express that.
> + fwnode_get_dependencies(child, deps);
> +}
> +
> +/**
> * fwnode_driver_match_device - Tell if a driver matches a device.
> * @drv: the device_driver structure to test
> * @dev: the device structure to match against
> diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> index 0408545..68ab558 100644
> --- a/include/linux/fwnode.h
> +++ b/include/linux/fwnode.h
> @@ -24,4 +24,9 @@ struct fwnode_handle {
> struct fwnode_handle *secondary;
> };
>
> +struct fwnode_dependency {
> + struct fwnode_handle *fwnode;
> + struct list_head dependency;
So this is a node in the list of dependencies, right?
I'd call that field "list_node", then.
> +};
And fwnode is a firmware node that the owner of the list depends on, right?
> +
> #endif
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 4e453c4..b8b86ea 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -86,6 +86,18 @@ bool fwnode_is_compatible(struct fwnode_handle *fwnode, const char *compatible);
> bool fwnode_driver_match_device(struct device *dev,
> const struct device_driver *drv);
>
> +void fwnode_add_dependency(struct fwnode_handle *fwnode,
> + struct list_head *list);
> +
> +void fwnode_add_dependency_parser(
> + void (*func)(struct fwnode_handle *fwnode, struct list_head *deps));
> +
> +void fwnode_remove_dependency_parser(
> + void (*func)(struct fwnode_handle *fwnode, struct list_head *deps));
> +
> +void fwnode_get_dependencies(struct fwnode_handle *fwnode,
> + struct list_head *list);
> +
> unsigned int device_get_child_node_count(struct device *dev);
>
> static inline bool device_property_read_bool(struct device *dev,
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: [PATCH v3] video-lp8788: Delete a check before backlight_device_unregister()
From: Jingoo Han @ 2015-07-02 4:44 UTC (permalink / raw)
To: SF Markus Elfring
Cc: Lee Jones, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
linux-fbdev@vger.kernel.org, Linux Kernel Mailing List,
kernel-janitors@vger.kernel.org, Julia Lawall, jingoo1han
In-Reply-To: <5593C14A.8020700@users.sourceforge.net>
> On 2015. 7. 1., at PM 7:30, SF Markus Elfring <elfring@users.sourceforge.net> wrote:
>
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 1 Jul 2015 12:08:31 +0200
>
> The backlight_device_unregister() function tests whether its argument
> is NULL and then returns immediately.
> Thus the test around the call is not needed.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
It looks good.
Acked-by: Jingoo Han <jingoohan1@gmail.com>
> ---
> drivers/video/backlight/lp8788_bl.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/video/backlight/lp8788_bl.c b/drivers/video/backlight/lp8788_bl.c
> index e418d5b..5d583d7 100644
> --- a/drivers/video/backlight/lp8788_bl.c
> +++ b/drivers/video/backlight/lp8788_bl.c
> @@ -221,8 +221,7 @@ static void lp8788_backlight_unregister(struct lp8788_bl *bl)
> {
> struct backlight_device *bl_dev = bl->bl_dev;
>
> - if (bl_dev)
> - backlight_device_unregister(bl_dev);
> + backlight_device_unregister(bl_dev);
> }
>
> static ssize_t lp8788_get_bl_ctl_mode(struct device *dev,
> --
> 2.4.5
>
^ permalink raw reply
* Re: [RFC PATCH 05/15] pwm: introduce default period and polarity concepts
From: Uwe Kleine-König @ 2015-07-02 6:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1435738921-25027-6-git-send-email-boris.brezillon@free-electrons.com>
On Wed, Jul 01, 2015 at 10:21:51AM +0200, Boris Brezillon wrote:
> When requested by a user, the PWM is assigned a default period and polarity
> extracted from the DT, the platform data or statically set by the driver.
> Those default values are currently stored in the period and polarity
> fields of the pwm_device struct, but they will be stored somewhere else
> once we have introduced the architecture allowing for hardware state
> retrieval.
>
> The pwm_set_default_polarity and pwm_set_default_period should only be
> used by PWM drivers or the PWM core infrastructure to specify the
> default period and polarity values.
Would it make sense to put the prototypes of
pwm_set_default_p{olarity,eriod} into (say) drivers/pwm/pwm-private.h
then?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [RFC PATCH 00/15] pwm: add support for atomic update
From: Uwe Kleine-König @ 2015-07-02 7:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1435738921-25027-1-git-send-email-boris.brezillon@free-electrons.com>
Hello Boris,
On Wed, Jul 01, 2015 at 10:21:46AM +0200, Boris Brezillon wrote:
> This series adds support for atomic PWM update, or ITO, the capability
> to update all the parameters of a PWM device (enabled/disabled, period,
> duty and polarity) in one go.
on first reading the subject of your series I thought it was about
asserting that the newly set config is active before the call to
pwm_config (et al) returns. That's a problem I addressed a few times in
the past. I wonder if it's only me or if a different wording should be
used for "update all parameters with a single function call".
But other than that I like the series.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [RFC PATCH 00/15] pwm: add support for atomic update
From: Tomi Valkeinen @ 2015-07-02 7:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150702070343.GD11824@pengutronix.de>
[-- Attachment #1: Type: text/plain, Size: 822 bytes --]
On 02/07/15 10:03, Uwe Kleine-König wrote:
> Hello Boris,
>
> On Wed, Jul 01, 2015 at 10:21:46AM +0200, Boris Brezillon wrote:
>> This series adds support for atomic PWM update, or ITO, the capability
>> to update all the parameters of a PWM device (enabled/disabled, period,
>> duty and polarity) in one go.
> on first reading the subject of your series I thought it was about
> asserting that the newly set config is active before the call to
> pwm_config (et al) returns. That's a problem I addressed a few times in
> the past. I wonder if it's only me or if a different wording should be
> used for "update all parameters with a single function call".
In my vocabulary "blocking" means that the work is done before the
function returns, and "atomic" means the work is done in one step.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 00/15] pwm: add support for atomic update
From: Boris Brezillon @ 2015-07-02 7:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150702070343.GD11824@pengutronix.de>
Hi Uwe,
On Thu, 2 Jul 2015 09:03:43 +0200
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> Hello Boris,
>
> On Wed, Jul 01, 2015 at 10:21:46AM +0200, Boris Brezillon wrote:
> > This series adds support for atomic PWM update, or ITO, the capability
> > to update all the parameters of a PWM device (enabled/disabled, period,
> > duty and polarity) in one go.
> on first reading the subject of your series I thought it was about
> asserting that the newly set config is active before the call to
> pwm_config (et al) returns. That's a problem I addressed a few times in
> the past. I wonder if it's only me or if a different wording should be
> used for "update all parameters with a single function call".
Yep, I can reword it differently.
>
> But other than that I like the series.
Great!
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [RFC PATCH 00/15] pwm: add support for atomic update
From: Uwe Kleine-König @ 2015-07-02 7:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5594E56C.3010908@ti.com>
On Thu, Jul 02, 2015 at 10:17:00AM +0300, Tomi Valkeinen wrote:
>
>
> On 02/07/15 10:03, Uwe Kleine-König wrote:
> > Hello Boris,
> >
> > On Wed, Jul 01, 2015 at 10:21:46AM +0200, Boris Brezillon wrote:
> >> This series adds support for atomic PWM update, or ITO, the capability
> >> to update all the parameters of a PWM device (enabled/disabled, period,
> >> duty and polarity) in one go.
> > on first reading the subject of your series I thought it was about
> > asserting that the newly set config is active before the call to
> > pwm_config (et al) returns. That's a problem I addressed a few times in
> > the past. I wonder if it's only me or if a different wording should be
> > used for "update all parameters with a single function call".
>
> In my vocabulary "blocking" means that the work is done before the
> function returns, and "atomic" means the work is done in one step.
blocking is IMHO something slightly different, maybe "synchronous" is a
good term for "done when the call returns".
For write(2) I'd say
- blocking means to only return when the write request has reached the
kernel, but not necessarily the medium. I.e. the caller doesn't need
to care further; and
- atomic means that the contents of two concurrent writers don't mix in
the resulting file content; and
- synchronous means that once write() returns the data is on the
medium.
So atomic seems to be fine to use here.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [RFC PATCH 13/15] pwm: rockchip: add support for atomic update
From: Boris Brezillon @ 2015-07-02 7:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1709826.dVqTms8REP@diego>
Hi Heiko,
On Wed, 01 Jul 2015 23:48:31 +0200
Heiko Stübner <heiko@sntech.de> wrote:
> Hi Boris,
>
>
> Am Mittwoch, 1. Juli 2015, 10:21:59 schrieb Boris Brezillon:
> > Implement the ->apply() function to add support for atomic update.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> > @@ -110,6 +113,26 @@ static void rockchip_pwm_set_enable_v2(struct pwm_chip
> > *chip, writel_relaxed(val, pc->base + pc->data->regs.ctrl);
> > }
> >
> > +static void rockchip_pwm_init_v2(struct pwm_chip *chip, struct pwm_device
> > *pwm) +{
> > + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
> > + u32 enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | PWM_ENABLE |
> > + PWM_CONTINUOUS;
> > + u32 val;
> > +
> > + val = readl(pc->base + pc->data->regs.ctrl);
> > +
> > + if ((val & enable_conf) != enable_conf)
> > + return;
> > +
> > + pwm->state.enabled = true;
> > +
> > + enable_conf = PWM_DUTY_NEGATIVE | PWM_INACTIVE_POSITIVE;
> > +
> > + if ((val & enable_conf) = enable_conf)
> > + pwm->state.polarity = PWM_POLARITY_INVERSED;
>
> the inactive setting does not affect the polarity of the running pwm, only what
> to do when it gets turned off. Also PWM_DUTY_NEGATIVE is the "0" value for the
> bit so also is bad to compare against (and results in wrong readings). So I
> would suggest changing this like
Indeed
>
> - enable_conf = PWM_DUTY_NEGATIVE | PWM_INACTIVE_POSITIVE;
> + enable_conf = PWM_DUTY_POSITIVE;
>
> - if ((val & enable_conf) = enable_conf)
> + if ((val & enable_conf) != enable_conf)
Or just:
if(val & PWM_DUTY_POSITIVE)
Thanks for the fix.
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [RFC PATCH 12/15] pwm: rockchip: add initial state retrieval
From: Boris Brezillon @ 2015-07-02 7:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <3258341.kaLi8uDfLr@diego>
On Wed, 01 Jul 2015 23:44:46 +0200
Heiko Stübner <heiko@sntech.de> wrote:
> Hi Boris,
>
> Am Mittwoch, 1. Juli 2015, 10:21:58 schrieb Boris Brezillon:
> > Implement the ->init_state() function to expose initial state.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
>
> [...]
>
> > @@ -98,6 +110,36 @@ static void rockchip_pwm_set_enable_v2(struct pwm_chip
> > *chip, writel_relaxed(val, pc->base + pc->data->regs.ctrl);
> > }
> >
> > +static void rockchip_pwm_init_state(struct pwm_chip *chip,
> > + struct pwm_device *pwm)
> > +{
> > + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
> > + unsigned long clk_rate;
> > + u64 tmp;
> > + int ret;
> > +
> > + ret = clk_enable(pc->clk);
> > + if (ret)
> > + return;
> > +
> > + clk_rate = clk_get_rate(pc->clk);
> > +
> > + tmp = readl(pc->base + pc->data->regs.period);
> > + tmp *= pc->data->prescaler * NSEC_PER_SEC;
> > + tmp = do_div(tmp, clk_rate);
>
> I guess you want to have the division result here and not the remainder, so
> - tmp = do_div(tmp, clk_rate);
> + do_div(tmp, clk_rate);
>
Oh crap. I make the same mistake over and over again.
[...]
> > static const struct rockchip_pwm_data pwm_data_v2 = {
> > @@ -207,6 +252,7 @@ static const struct rockchip_pwm_data pwm_data_v2 = {
> > .prescaler = 1,
> > .ops = &rockchip_pwm_ops_v2,
> > .set_enable = rockchip_pwm_set_enable_v2,
> > + .init = rockchip_pwm_init_v2,
>
> you're referencing the v2 init here, but only add it in the next patch?
> [pwm: rockchip: add support for atomic update]
Yep, this function should be added in this patch.
Thanks,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [RFC PATCH 05/15] pwm: introduce default period and polarity concepts
From: Boris Brezillon @ 2015-07-02 7:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150702064445.GC11824@pengutronix.de>
On Thu, 2 Jul 2015 08:44:45 +0200
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> On Wed, Jul 01, 2015 at 10:21:51AM +0200, Boris Brezillon wrote:
> > When requested by a user, the PWM is assigned a default period and polarity
> > extracted from the DT, the platform data or statically set by the driver.
> > Those default values are currently stored in the period and polarity
> > fields of the pwm_device struct, but they will be stored somewhere else
> > once we have introduced the architecture allowing for hardware state
> > retrieval.
> >
> > The pwm_set_default_polarity and pwm_set_default_period should only be
> > used by PWM drivers or the PWM core infrastructure to specify the
> > default period and polarity values.
> Would it make sense to put the prototypes of
> pwm_set_default_p{olarity,eriod} into (say) drivers/pwm/pwm-private.h
> then?
>
Yes, definitely. I was thinking about moving those functions/prototypes
into include/linux/pwm-provider.h, but I'm fine with
drivers/pwm/pwm-private.h too.
Thierry, any opinion ?
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [RFC PATCH 00/15] pwm: add support for atomic update
From: Boris Brezillon @ 2015-07-02 7:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <3250976.mr9RMtjrdC@diego>
On Wed, 01 Jul 2015 23:57:57 +0200
Heiko Stübner <heiko@sntech.de> wrote:
> Hi Boris,
>
> Am Mittwoch, 1. Juli 2015, 10:21:46 schrieb Boris Brezillon:
> > Hello Thierry,
> >
> > This series adds support for atomic PWM update, or ITO, the capability
> > to update all the parameters of a PWM device (enabled/disabled, period,
> > duty and polarity) in one go.
> >
> > This implementation is still experimental, and I may have missed some key
> > aspect, so any feedback are welcome.
> >
> > Also note that I haven't protected the state update with any locking.
> > That's because the existing config does not protect against concurrent
> > access to a requested PWM device (see the pwm_config implementation).
> > I guess the PWM framework assume the user will implement the proper locking
> > scheme if it has to concurrently access the device.
> >
> > The 5 first patches prepare the addition of the pwm_state concept, which
> > will be used to allow atomic updates.
> > The following patches introduce the pwm_state struct, initial state
> > retrieval and atomic update concepts.
> >
> > Patches 12 and 13 are showing how one can implement the initial state
> > retrieval and atomic update features in a PWM driver (in this specific
> > case I implemented it in the rockchip driver).
> >
> > The last 2 patches are making use of those changes to improve the
> > pwm-regulator driver (initializing the regulator state based on the
> > initial PWM state).
>
> at first I got very strange readings (very wrong values and wrong polarity),
> which resulted from the issues I pointed out in the replies to individual
> patches. After fixing these, the pwm read-back now returns exactly the expected
> values :-) .
Sorry about that, as I said I only compile tested the series :-/.
Anyway, thanks for providing fixes for these bugs, they'll be applied
in the next version.
>
> And with the original voltage table from the Chromeos-devicetrees, the pwm-
> regulator also returns the expected 1.2V that coreboot initially set.
Great! And thanks for testing the patches.
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [RFC PATCH 16/15] pwm: add informations about polarity, duty cycle and period to debugfs
From: Boris Brezillon @ 2015-07-02 13:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <3365087.n2eEES60JK@diego>
On Wed, 01 Jul 2015 23:50:46 +0200
Heiko Stübner <heiko@sntech.de> wrote:
> The pwm-states make it possible to also output the polarity, duty cycle
> and period information in the debugfs pwm summary-outout.
> This makes it easier to gather overview information about pwms without
> needing to walk through the sysfs attributes of every pwm.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
> might be nice to have too ;-)
Yes.
>
> drivers/pwm/core.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 6dafd8e..79037a2 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -951,9 +951,18 @@ static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
> if (test_bit(PWMF_REQUESTED, &pwm->flags))
> seq_puts(s, " requested");
>
> - if (pwm_is_enabled(pwm))
> + if (pwm_is_enabled(pwm)) {
> seq_puts(s, " enabled");
>
> + seq_printf(s, " period:%uns",
> + pwm_get_period(pwm));
> + seq_printf(s, " duty:%uns",
> + pwm_get_duty_cycle(pwm));
> + seq_printf(s, " polarity:%s",
> + pwm_get_polarity(pwm) ? "inverse"
> + : "normal");
I would print those values even if the PWM is not enabled.
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH v7 5/9] PCI: Add pci_iomap_wc() variants
From: Luis R. Rodriguez @ 2015-07-02 18:49 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Casey Leedom, Arnd Bergmann, Michael S. Tsirkin, Bjorn Helgaas,
Toshi Kani, Andy Lutomirski, Juergen Gross, Tomi Valkeinen,
linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
xen-devel@lists.xensource.com, linux-fbdev, Suresh Siddha,
Ingo Molnar, Thomas Gleixner, Daniel Vetter, Dave Airlie,
Antonino Daplas, Jean-Christophe Plagniol-Villard, Dave Hansen,
venkatesh.pallipadi@intel.com, Stefan Bader,
ville.syrjala@linux.intel.com, David Vrabel, Jan Beulich,
Roger Pau Monné
In-Reply-To: <1435356048.26815.9.camel@kernel.crashing.org>
On Sat, Jun 27, 2015 at 08:00:48AM +1000, Benjamin Herrenschmidt wrote:
> On Fri, 2015-06-26 at 16:24 +0000, Casey Leedom wrote:
> > Thanks for looking into this Ben. As it stands now, it seems as
> > if Write Combined mappings simply aren't supported and/or all
> > driver writers trying to utilize Write Combined mappings have to
> > "hand roll" their own solutions which really isn't supportable.
> >
> > One thing that might be considered is simply to treat the desire
> > to utilize the Write Combining hardware as a separate issue and
> > develop writel_wc(), writeq_wc(), etc.
That seems rather sloppy and cumbersome, its best to just provide the
infrastructure for initial mapping for an area and let the hardware do it for
you. With the current design drivers would just use regular read/write on all
areas and the only thing that will set them apart will be the mapping. With
what you propose we'd end up having to shift a whole bunch of reads/writes for
just the purpose of adding WC to an area that didn't have wc before. That's
a waste of code and time.
> > Those could be defined
> > as legal only for Write Combined Mappings and would "do the
> > right thing" for each architecture.
Yuck.
> The question then is what is "the right thing". In the powerpc case,
> we'll have a non-garded mapping, which means we also get no ordering
> between load and stores.
I don't follow, you *ordering* between load and stores for WC? We should
not need that for WC, its why WC is used for only very specific things
such as framebuffer and PIO (which BTw I still don't quite get all this
use case for infiniband to be honest, and I will note I do see some
proprietary hardware extensions like bursts but nothing covering all
this in a general doc, I think I think it all just has to do that this
is a hardware hack in reality, which we sell as a feature).
Luis
^ permalink raw reply
* Re: [PATCH v7 5/9] PCI: Add pci_iomap_wc() variants
From: Benjamin Herrenschmidt @ 2015-07-02 22:26 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Casey Leedom, Arnd Bergmann, Michael S. Tsirkin, Bjorn Helgaas,
Toshi Kani, Andy Lutomirski, Juergen Gross, Tomi Valkeinen,
linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
xen-devel@lists.xensource.com, linux-fbdev, Suresh Siddha,
Ingo Molnar, Thomas Gleixner, Daniel Vetter, Dave Airlie,
Antonino Daplas, Jean-Christophe Plagniol-Villard, Dave Hansen,
venkatesh.pallipadi@intel.com, Stefan Bader,
ville.syrjala@linux.intel.com, David Vrabel, Jan Beulich,
Roger Pau Monné
In-Reply-To: <20150702184904.GA7021@wotan.suse.de>
On Thu, 2015-07-02 at 20:49 +0200, Luis R. Rodriguez wrote:
> > The question then is what is "the right thing". In the powerpc case,
> > we'll have a non-garded mapping, which means we also get no ordering
> > between load and stores.
>
> I don't follow, you *ordering* between load and stores for WC? We should
> not need that for WC, its why WC is used for only very specific things
> such as framebuffer and PIO (which BTw I still don't quite get all this
> use case for infiniband to be honest, and I will note I do see some
> proprietary hardware extensions like bursts but nothing covering all
> this in a general doc, I think I think it all just has to do that this
> is a hardware hack in reality, which we sell as a feature).
Well, that's the problem, the semantics that we provide to drivers
aren't well defined.
The words "write combine" themselves only specify that writes to subsequent
addresses can be combined into larger transactions. That's in itself is already
quite vague (are their boundaries, limits ? some can depend on bus type, etc...)
though in practice is probably sufficient.
However, overloading a _wc mapping with additional memory model differences
such as loss of ordering between load and stores, etc... is not an obvious
thing to do. I agree it would make *my* life easier if we did it since this
is precisely the semantics provided by a "G=0" mapping on ppc, but we need
to agree and *document* it, otherwise bad things will happen eventually.
We also need to document in that case which barriers can be used to explicitly
enforce the ordering on such a mapping and which barriers can be used to break
write combine (they don't necessarily have to be the same).
We also need to document which accessors will actually provide the write combine
"feature" of a _wc mapping. For example while writel() will do it on Intel, it
will not on ppc and I wouldn't be surprised if a bunch of other archs fall in
the same bucket as ppc (basically anything that has barriers in their writel
implementation to order vs. DMA etc...).
So we might need to explicitly document that writel_relaxed() needs to be used.
Finally what are the precise guaranteed semantics of writeX/readX,
writeX_relaxed/readX_relaxed and __raw_ (everything else) on a _wc mapping, what
do we mandate and document, what do we leave to be implementation dependent ?
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH v5 1/3] video: fbdev: atyfb: clarify ioremap() base and length used
From: Luis R. Rodriguez @ 2015-07-02 23:23 UTC (permalink / raw)
To: Borislav Petkov
Cc: Ville Syrjälä, Andrew Morton, Ingo Molnar,
Ville Syrjälä, Andy Lutomirski, Tomi Valkeinen,
Michael S. Tsirkin, Benjamin Herrenschmidt,
linux-kernel@vger.kernel.org, linux-fbdev,
linux-pci@vger.kernel.org, Dave Airlie, Toshi Kani, Suresh Siddha,
Linus Torvalds, Thomas Gleixner, Juergen Gross, Daniel Vetter,
Antonino Daplas, Jean-Christophe Plagniol-Villard, Rob Clark,
Mathias Krause, Andrzej Hajda, Mel Gorman, Vlastimil Babka,
Davidlohr Bueso
In-Reply-To: <20150626073028.GA14776@pd.tnic>
On Fri, Jun 26, 2015 at 12:30 AM, Borislav Petkov <bp@suse.de> wrote:
> On Fri, Jun 26, 2015 at 03:09:27AM +0200, Luis R. Rodriguez wrote:
>> Sure, mind this as a follow up patch if its too late?
>
> No need, you can send me an updated one - I'll replace it.
Will do!
Luis
^ permalink raw reply
* Re: [PATCH v7 5/9] PCI: Add pci_iomap_wc() variants
From: Casey Leedom @ 2015-07-03 0:14 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Benjamin Herrenschmidt, Arnd Bergmann, Michael S. Tsirkin,
Bjorn Helgaas, Toshi Kani, Andy Lutomirski, Juergen Gross,
Tomi Valkeinen, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org, xen-devel@lists.xensource.com,
linux-fbdev, Suresh Siddha, Ingo Molnar, Thomas Gleixner,
Daniel Vetter, Dave Airlie, Antonino Daplas,
Jean-Christophe Plagniol-Villard, Dave Hansen,
venkatesh.pallipadi@intel.com, Stefan Bader,
ville.syrjala@linux.intel.com, David Vrabel, Jan Beulich,
Roger Pau Monné
In-Reply-To: <20150702184904.GA7021@wotan.suse.de>
> On Jul 2, 2015, at 11:49 AM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>
> On Sat, Jun 27, 2015 at 08:00:48AM +1000, Benjamin Herrenschmidt wrote:
>> On Fri, 2015-06-26 at 16:24 +0000, Casey Leedom wrote:
>>> Thanks for looking into this Ben. As it stands now, it seems as
>>> if Write Combined mappings simply aren't supported and/or all
>>> driver writers trying to utilize Write Combined mappings have to
>>> "hand roll" their own solutions which really isn't supportable.
>>>
>>> One thing that might be considered is simply to treat the desire
>>> to utilize the Write Combining hardware as a separate issue and
>>> develop writel_wc(), writeq_wc(), etc.
>
> That seems rather sloppy and cumbersome, its best to just provide the
> infrastructure for initial mapping for an area and let the hardware do it for
> you. With the current design drivers would just use regular read/write on all
> areas and the only thing that will set them apart will be the mapping. With
> what you propose we'd end up having to shift a whole bunch of reads/writes for
> just the purpose of adding WC to an area that didn't have wc before. That's
> a waste of code and time.
>
>>> Those could be defined
>>> as legal only for Write Combined Mappings and would "do the
>>> right thing" for each architecture.
>
> Yuck.
Yeah, probably. But it sounds like from Ben’s response, we should really use write_relaxed() as an already existing API for this. But most of the architectures just define this as writel() so some work would need to be done to get it to work.
>> The question then is what is "the right thing". In the powerpc case,
>> we'll have a non-garded mapping, which means we also get no ordering
>> between load and stores.
>
> I don't follow, you *ordering* between load and stores for WC? We should
> not need that for WC, its why WC is used for only very specific things
> such as framebuffer and PIO (which BTw I still don't quite get all this
> use case for infiniband to be honest, and I will note I do see some
> proprietary hardware extensions like bursts but nothing covering all
> this in a general doc, I think I think it all just has to do that this
> is a hardware hack in reality, which we sell as a feature).
I was talking about the work that our drivers (cxgb4, cxgb4vf, etc.) do to ensure correct ordering between writes to memory and I/O space. For instance, issuing a wmb() between writes to a DMA buffer and the write to a register telling the hardware that the data is available. This isn’t necessary on the Strongly Ordered Intel architectures but is necessary on other architectures.
Casey
^ permalink raw reply
* [GIT PULL] fbdev fixes for 4.2
From: Tomi Valkeinen @ 2015-07-03 8:16 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-fbdev, linux-kernel@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 809 bytes --]
Hi Linus,
The following changes since commit f778dad38a54ca5207d024b5ebe0e6d71b8bad83:
Merge omapdss scaling fixes (2015-06-22 14:56:01 +0300)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/tomba/linux.git tags/fbdev-fixes-4.2
for you to fetch changes up to 0438ec90b17963662ef619b71c77c5c135b07014:
OMAPDSS: fix probing if rfbi device is enabled (2015-07-02 15:20:10 +0300)
----------------------------------------------------------------
fbdev fixes for 4.2
* Fix display regression on TI AM4xxx boards
----------------------------------------------------------------
Tomi Valkeinen (1):
OMAPDSS: fix probing if rfbi device is enabled
drivers/video/fbdev/omap2/dss/dss.c | 9 +++++++++
1 file changed, 9 insertions(+)
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* [PATCH 0/3] video/logo: add option to return largest logo for a given resolution
From: Urs Fässler @ 2015-07-03 8:38 UTC (permalink / raw)
To: daniel.vetter, linux-fbdev, linux-kernel, oliver.staebler,
plagnioj, tomi.valkeinen, urs.fassler
With this patches, the largest boot logo for a given display resolution is used.
We need that functionality since it is common that a product has variants which
differ only minimal in the hardware. Thus, we have the same kernel binary but
different device trees for different hardware / displays.
This patches allow use to show a full screen boot logo for every hardware
variant.
Urs Fässler (3):
video/logo: use list of logos to find logo
logo: add option to return largest logo for a given resolution
fbdev: use largest logo if possible
drivers/video/fbdev/core/fbmem.c | 21 ++++--
drivers/video/logo/Kconfig | 7 ++
drivers/video/logo/logo.c | 146 ++++++++++++++++++++++++++-------------
include/linux/linux_logo.h | 4 ++
4 files changed, 124 insertions(+), 54 deletions(-)
--
2.1.4
^ 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