* [PATCH 4/8] PM / Domains: Add fwnode provider to genpd states
From: Lina Iyer @ 2016-10-06 15:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAPDyKFrsLjchkbkqg4O=RfKamO+EgOhZtzw1-_3e5QphLON0XQ@mail.gmail.com>
On Thu, Oct 06 2016 at 06:01 -0600, Ulf Hansson wrote:
>On 5 October 2016 at 22:31, Lina Iyer <lina.iyer@linaro.org> wrote:
>> Save the fwnode for the genpd state in the state node. PM Domain clients
>> may use the fwnode to read in the rest of the properties for the domain
>> state.
>
>What is the "rest"? I assume you mean the non PM domain generic parts,
>but perhaps you could elaborate a bit on that?
>
>>
>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>> ---
>> drivers/base/power/domain.c | 1 +
>> include/linux/pm_domain.h | 1 +
>> 2 files changed, 2 insertions(+)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 368a5b8..52fcdb2 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -1937,6 +1937,7 @@ static int read_genpd_state(struct genpd_power_state *genpd_state,
>> latency = entry_latency + exit_latency;
>> genpd_state->power_on_latency_ns = 1000 * latency;
>> genpd_state->power_off_latency_ns = 1000 * entry_latency;
>> + genpd_state->provider = &state_node->fwnode;
>>
>> return 0;
>> }
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index 4c9152d..eacfa71 100644
>> --- a/include/linux/pm_domain.h
>> +++ b/include/linux/pm_domain.h
>> @@ -39,6 +39,7 @@ struct genpd_power_state {
>> s64 power_off_latency_ns;
>> s64 power_on_latency_ns;
>> s64 residency_ns;
>> + struct fwnode_handle *provider;
>
>I don't think this is a provider, but just a fwnode_handle to a
>domain-idle-state. Therefore, I would suggest you to rename it to
>"fwnode" instead.
>
OK.
>> };
>>
>> struct generic_pm_domain {
>> --
>> 2.7.4
>>
>
>Kind regards
>Uffe
^ permalink raw reply
* [PATCH 3/8] PM / Domains: Allow domain power states to be read from DT
From: Lina Iyer @ 2016-10-06 15:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAPDyKFqWE=-tMLN-6zKWpb6K2a2bBxkA7q+mSivJFpzNntcupw@mail.gmail.com>
On Thu, Oct 06 2016 at 03:47 -0600, Ulf Hansson wrote:
>On 5 October 2016 at 22:31, Lina Iyer <lina.iyer@linaro.org> wrote:
>> This patch allows domains to define idle states in the DT. SoC's can
>> define domain idle states in DT using the "domain-idle-states" property
>> of the domain provider. Calling of_pm_genpd_init() will read the idle
>> states and initialize the genpd for the domain.
>>
>> In addition to the entry and exit latency for idle state, also add
>> residency_ns, param and of_node property to each state. A domain idling
>> in a state is only power effecient if it stays idle for a certain period
>> in that state. The residency provides this minimum time for the idle
>> state to provide power benefits. The param is a state specific u32 value
>> that the platform may use for that idle state.
>>
>> This patch is based on the original patch by Marc Titinger.
>>
>> Signed-off-by: Marc Titinger <mtitinger+renesas@baylibre.com>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>> ---
>> drivers/base/power/domain.c | 103 ++++++++++++++++++++++++++++++++++++++++++++
>> include/linux/pm_domain.h | 8 ++++
>> 2 files changed, 111 insertions(+)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 740afa9..368a5b8 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -1895,6 +1895,109 @@ out:
>> return ret ? -EPROBE_DEFER : 0;
>> }
>> EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
>> +
>> +static const struct of_device_id idle_state_match[] = {
>> + { .compatible = "arm,idle-state", },
>> + { }
>> +};
>> +
>> +static int read_genpd_state(struct genpd_power_state *genpd_state,
>
>/s/read_genpd_state/genpd_parse_state
>
Ok.
>> + struct device_node *state_node)
>> +{
>> + int err = 0;
>
>No need to assign err to 0.
>
Will fix.
>> + u32 latency;
>> + u32 residency;
>> + u32 entry_latency, exit_latency;
>> + const struct of_device_id *match_id;
>> +
>> + match_id = of_match_node(idle_state_match, state_node);
>> + if (!match_id)
>> + return -EINVAL;
>> +
>> + err = of_property_read_u32(state_node, "entry-latency-us",
>> + &entry_latency);
>> + if (err) {
>> + pr_debug(" * %s missing entry-latency-us property\n",
>> + state_node->full_name);
>> + return -EINVAL;
>> + }
>> +
>> + err = of_property_read_u32(state_node, "exit-latency-us",
>> + &exit_latency);
>> + if (err) {
>> + pr_debug(" * %s missing exit-latency-us property\n",
>> + state_node->full_name);
>> + return -EINVAL;
>> + }
>> +
>> + err = of_property_read_u32(state_node, "min-residency-us", &residency);
>> + if (!err)
>> + genpd_state->residency_ns = 1000 * residency;
>> +
>> + latency = entry_latency + exit_latency;
>
>Hmm, this is probably not what you want.
>
>The genpd governor, via __default_power_down_ok(), already adds the
>->power_on_latency_ns and the ->power_off_latency_ns, when it
>validates which idle state you are allowed to enter.
>
Good catch.
>> + genpd_state->power_on_latency_ns = 1000 * latency;
>> + genpd_state->power_off_latency_ns = 1000 * entry_latency;
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * of_genpd_parse_idle_states: Return array of idle states for the genpd.
>> + *
>> + * @dn: The genpd device node
>> + * @states: The pointer to which the state array will be saved.
>> + * @n: The count of elements in the array returned from this function.
>> + *
>> + * Returns the device states parsed from the OF node. The memory for the states
>> + * is allocated by this function and is the responsibility of the caller to
>> + * free the memory after use.
>> + */
>> +int of_genpd_parse_idle_states(struct device_node *dn,
>> + struct genpd_power_state **states, int *n)
>> +{
>> + struct genpd_power_state *st;
>> + struct device_node *np;
>> + int i, ret = 0;
>> + int count;
>> +
>> + for (count = 0; ; count++)
>> + if (!of_parse_phandle(dn, "domain-idle-states", count))
>> + break;
>
>I think it's better to use of_count_phandle_with_args() to find out
>the number of phandles.
>
>> +
>> + st = kcalloc(count, sizeof(*st), GFP_KERNEL);
>> + if (!st)
>> + return -ENOMEM;
>> +
>> + for (i = 0; i < count; i++) {
>> + np = of_parse_phandle(dn, "domain-idle-states", i);
>
>Isn't this a case of when it would be convenient to use the
>of_phandle_iterator*() APIs?
>
>> + if (!np) {
>> + ret = -EFAULT;
>> + break;
>> + }
>> +
>> + ret = read_genpd_state(&st[i], np);
>> + if (ret) {
>> + pr_err
>> + ("Parsing idle state node %s failed with err %d\n",
>> + np->full_name, ret);
>> + of_node_put(np);
>> + break;
>> + }
>> + of_node_put(np);
>> + }
>> +
>> + if (ret) {
>> + kfree(st);
>> + return ret;
>> + }
>> +
>> + *n = count;
>> + *states = st;
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(of_genpd_parse_idle_states);
>> +
>> #endif /* CONFIG_PM_GENERIC_DOMAINS_OF */
>>
>>
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index c113713..4c9152d 100644
>> --- a/include/linux/pm_domain.h
>> +++ b/include/linux/pm_domain.h
>> @@ -204,6 +204,8 @@ extern int of_genpd_add_device(struct of_phandle_args *args,
>> extern int of_genpd_add_subdomain(struct of_phandle_args *parent,
>> struct of_phandle_args *new_subdomain);
>> extern struct generic_pm_domain *of_genpd_remove_last(struct device_node *np);
>> +extern int of_genpd_parse_idle_states(struct device_node *dn,
>> + struct genpd_power_state **states, int *n);
>>
>> int genpd_dev_pm_attach(struct device *dev);
>> #else /* !CONFIG_PM_GENERIC_DOMAINS_OF */
>> @@ -233,6 +235,12 @@ static inline int of_genpd_add_subdomain(struct of_phandle_args *parent,
>> return -ENODEV;
>> }
>>
>> +static inline int of_genpd_parse_idle_states(struct device_node *dn,
>> + struct genpd_power_state **states, int *n)
>> +{
>> + return -ENODEV;
>> +}
>> +
>> static inline int genpd_dev_pm_attach(struct device *dev)
>> {
>> return -ENODEV;
>> --
>> 2.7.4
>>
>
>Kind regards
>Uffe
^ permalink raw reply
* [PATCH 3/8] PM / Domains: Allow domain power states to be read from DT
From: Lina Iyer @ 2016-10-06 15:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAPDyKFqWE=-tMLN-6zKWpb6K2a2bBxkA7q+mSivJFpzNntcupw@mail.gmail.com>
On Thu, Oct 06 2016 at 03:47 -0600, Ulf Hansson wrote:
>On 5 October 2016 at 22:31, Lina Iyer <lina.iyer@linaro.org> wrote:
>> This patch allows domains to define idle states in the DT. SoC's can
>> define domain idle states in DT using the "domain-idle-states" property
>> of the domain provider. Calling of_pm_genpd_init() will read the idle
>> states and initialize the genpd for the domain.
>>
>> In addition to the entry and exit latency for idle state, also add
>> residency_ns, param and of_node property to each state. A domain idling
>> in a state is only power effecient if it stays idle for a certain period
>> in that state. The residency provides this minimum time for the idle
>> state to provide power benefits. The param is a state specific u32 value
>> that the platform may use for that idle state.
>>
>> This patch is based on the original patch by Marc Titinger.
>>
>> Signed-off-by: Marc Titinger <mtitinger+renesas@baylibre.com>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>> ---
>> drivers/base/power/domain.c | 103 ++++++++++++++++++++++++++++++++++++++++++++
>> include/linux/pm_domain.h | 8 ++++
>> 2 files changed, 111 insertions(+)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 740afa9..368a5b8 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -1895,6 +1895,109 @@ out:
>> return ret ? -EPROBE_DEFER : 0;
>> }
>> EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
>> +
>> +static const struct of_device_id idle_state_match[] = {
>> + { .compatible = "arm,idle-state", },
>> + { }
>> +};
>> +
>> +static int read_genpd_state(struct genpd_power_state *genpd_state,
>
>/s/read_genpd_state/genpd_parse_state
>
>> + struct device_node *state_node)
>> +{
>> + int err = 0;
>
>No need to assign err to 0.
>
>> + u32 latency;
>> + u32 residency;
>> + u32 entry_latency, exit_latency;
>> + const struct of_device_id *match_id;
>> +
>> + match_id = of_match_node(idle_state_match, state_node);
>> + if (!match_id)
>> + return -EINVAL;
>> +
>> + err = of_property_read_u32(state_node, "entry-latency-us",
>> + &entry_latency);
>> + if (err) {
>> + pr_debug(" * %s missing entry-latency-us property\n",
>> + state_node->full_name);
>> + return -EINVAL;
>> + }
>> +
>> + err = of_property_read_u32(state_node, "exit-latency-us",
>> + &exit_latency);
>> + if (err) {
>> + pr_debug(" * %s missing exit-latency-us property\n",
>> + state_node->full_name);
>> + return -EINVAL;
>> + }
>> +
>> + err = of_property_read_u32(state_node, "min-residency-us", &residency);
>> + if (!err)
>> + genpd_state->residency_ns = 1000 * residency;
>> +
>> + latency = entry_latency + exit_latency;
>
>Hmm, this is probably not what you want.
>
>The genpd governor, via __default_power_down_ok(), already adds the
>->power_on_latency_ns and the ->power_off_latency_ns, when it
>validates which idle state you are allowed to enter.
>
>> + genpd_state->power_on_latency_ns = 1000 * latency;
>> + genpd_state->power_off_latency_ns = 1000 * entry_latency;
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * of_genpd_parse_idle_states: Return array of idle states for the genpd.
>> + *
>> + * @dn: The genpd device node
>> + * @states: The pointer to which the state array will be saved.
>> + * @n: The count of elements in the array returned from this function.
>> + *
>> + * Returns the device states parsed from the OF node. The memory for the states
>> + * is allocated by this function and is the responsibility of the caller to
>> + * free the memory after use.
>> + */
>> +int of_genpd_parse_idle_states(struct device_node *dn,
>> + struct genpd_power_state **states, int *n)
>> +{
>> + struct genpd_power_state *st;
>> + struct device_node *np;
>> + int i, ret = 0;
>> + int count;
>> +
>> + for (count = 0; ; count++)
>> + if (!of_parse_phandle(dn, "domain-idle-states", count))
>> + break;
>
>I think it's better to use of_count_phandle_with_args() to find out
>the number of phandles.
>
>> +
>> + st = kcalloc(count, sizeof(*st), GFP_KERNEL);
>> + if (!st)
>> + return -ENOMEM;
>> +
>> + for (i = 0; i < count; i++) {
>> + np = of_parse_phandle(dn, "domain-idle-states", i);
>
>Isn't this a case of when it would be convenient to use the
>of_phandle_iterator*() APIs?
>
Hmm.. But if we move back to static allocation of states, we won't need
any of this ;)
-- Lina
>> + if (!np) {
>> + ret = -EFAULT;
>> + break;
>> + }
>> +
>> + ret = read_genpd_state(&st[i], np);
>> + if (ret) {
>> + pr_err
>> + ("Parsing idle state node %s failed with err %d\n",
>> + np->full_name, ret);
>> + of_node_put(np);
>> + break;
>> + }
>> + of_node_put(np);
>> + }
>> +
>> + if (ret) {
>> + kfree(st);
>> + return ret;
>> + }
>> +
>> + *n = count;
>> + *states = st;
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(of_genpd_parse_idle_states);
>> +
>> #endif /* CONFIG_PM_GENERIC_DOMAINS_OF */
>>
>>
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index c113713..4c9152d 100644
>> --- a/include/linux/pm_domain.h
>> +++ b/include/linux/pm_domain.h
>> @@ -204,6 +204,8 @@ extern int of_genpd_add_device(struct of_phandle_args *args,
>> extern int of_genpd_add_subdomain(struct of_phandle_args *parent,
>> struct of_phandle_args *new_subdomain);
>> extern struct generic_pm_domain *of_genpd_remove_last(struct device_node *np);
>> +extern int of_genpd_parse_idle_states(struct device_node *dn,
>> + struct genpd_power_state **states, int *n);
>>
>> int genpd_dev_pm_attach(struct device *dev);
>> #else /* !CONFIG_PM_GENERIC_DOMAINS_OF */
>> @@ -233,6 +235,12 @@ static inline int of_genpd_add_subdomain(struct of_phandle_args *parent,
>> return -ENODEV;
>> }
>>
>> +static inline int of_genpd_parse_idle_states(struct device_node *dn,
>> + struct genpd_power_state **states, int *n)
>> +{
>> + return -ENODEV;
>> +}
>> +
>> static inline int genpd_dev_pm_attach(struct device *dev)
>> {
>> return -ENODEV;
>> --
>> 2.7.4
>>
>
>Kind regards
>Uffe
^ permalink raw reply
* [PATCH v3] Adding missing features of Coresight PTM components
From: Mathieu Poirier @ 2016-10-06 15:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <60a1f04c-1e73-0ff1-dfee-775b875d2c4a@centralesupelec.fr>
On 6 October 2016 at 06:18, Muhammad Abdul WAHAB
<muhammadabdul.wahab@centralesupelec.fr> wrote:
> In the current driver for Coresight components, two features of PTM
> components are missing:
>
> 1. Branch Broadcasting (present also in ETM but called Branch Output)
> 2. Return Stack (only present in PTM v1.0 and PTMv1.1)
>
> These features can be added simply to the code using `mode` field of
> `etm_config` struct.
>
> 1. **Branch Broadcast** : The branch broadcast feature is present in ETM
> components as well and is called Branch output. It allows to retrieve
> addresses for direct branch addresses alongside the indirect branch
> addresses. For example, it could be useful in cases when tracing without
> source code.
> 2. **Return Stack** : The return stack option allows to retrieve the return
> addresses of function calls. It can be useful to avoid CRA
> (Code Reuse Attacks) by keeping a shadowstack.
>
> Signed-off-by: Muhammad Abdul Wahab <muhammadabdul.wahab@centralesupelec.fr>
> ---
> changes in v3 :
> - deleted warning message
> - bit field definition of Branch Broadcast and Return Stack
>
> drivers/hwtracing/coresight/coresight-etm.h | 5 +++++
> drivers/hwtracing/coresight/coresight-etm3x-sysfs.c | 10 ++++++++++
> 2 files changed, 15 insertions(+)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm.h
> b/drivers/hwtracing/coresight/coresight-etm.h
> index 4a18ee4..ad063d7 100644
> --- a/drivers/hwtracing/coresight/coresight-etm.h
> +++ b/drivers/hwtracing/coresight/coresight-etm.h
> @@ -89,11 +89,13 @@
> /* ETMCR - 0x00 */
> #define ETMCR_PWD_DWN BIT(0)
> #define ETMCR_STALL_MODE BIT(7)
> +#define ETMCR_BRANCH_BROADCAST BIT(8)
> #define ETMCR_ETM_PRG BIT(10)
> #define ETMCR_ETM_EN BIT(11)
> #define ETMCR_CYC_ACC BIT(12)
> #define ETMCR_CTXID_SIZE (BIT(14)|BIT(15))
> #define ETMCR_TIMESTAMP_EN BIT(28)
> +#define ETMCR_RETURN_STACK BIT(29)
> /* ETMCCR - 0x04 */
> #define ETMCCR_FIFOFULL BIT(23)
> /* ETMPDCR - 0x310 */
> @@ -110,8 +112,11 @@
> #define ETM_MODE_STALL BIT(2)
> #define ETM_MODE_TIMESTAMP BIT(3)
> #define ETM_MODE_CTXID BIT(4)
> +#define ETM_MODE_BBROAD BIT(5)
> +#define ETM_MODE_RET_STACK BIT(6)
> #define ETM_MODE_ALL (ETM_MODE_EXCLUDE | ETM_MODE_CYCACC | \
> ETM_MODE_STALL | ETM_MODE_TIMESTAMP | \
> + ETM_MODE_BBROAD | ETM_MODE_RET_STACK | \
> ETM_MODE_CTXID | ETM_MODE_EXCL_KERN | \
> ETM_MODE_EXCL_USER)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> index 5ea0909..a76009a 100644
> --- a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> +++ b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> @@ -164,6 +164,16 @@ static ssize_t mode_store(struct device *dev,
> else
> config->ctrl &= ~ETMCR_CTXID_SIZE;
>
> + if (config->mode & ETM_MODE_BBROAD)
> + config->ctrl |= ETMCR_BRANCH_BROADCAST;
> + else
> + config->ctrl &= ~ETMCR_BRANCH_BROADCAST;
> +
> + if (config->mode & ETM_MODE_RET_STACK)
> + config->ctrl |= ETMCR_RETURN_STACK_EN;
> + else
> + config->ctrl &= ~ETMCR_RETURN_STACK_EN;
> +
Where is ETMCR_RETURN_STACK_EN defined? Did you send me code that
doesn't compile?
> if (config->mode & (ETM_MODE_EXCL_KERN | ETM_MODE_EXCL_USER))
> etm_config_trace_mode(config);
>
> --
> 1.9.1
^ permalink raw reply
* [PATCH 3/8] PM / Domains: Allow domain power states to be read from DT
From: Lina Iyer @ 2016-10-06 15:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAMuHMdU4HiauULf4fX67Cwj1GQmEExBiu4WyQPJBbj=xNg0eEQ@mail.gmail.com>
On Thu, Oct 06 2016 at 02:04 -0600, Geert Uytterhoeven wrote:
>Hi Lina,
>
>On Wed, Oct 5, 2016 at 10:31 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>> This patch allows domains to define idle states in the DT. SoC's can
>> define domain idle states in DT using the "domain-idle-states" property
>> of the domain provider. Calling of_pm_genpd_init() will read the idle
>> states and initialize the genpd for the domain.
>>
>> In addition to the entry and exit latency for idle state, also add
>> residency_ns, param and of_node property to each state. A domain idling
>> in a state is only power effecient if it stays idle for a certain period
>> in that state. The residency provides this minimum time for the idle
>> state to provide power benefits. The param is a state specific u32 value
>> that the platform may use for that idle state.
>>
>> This patch is based on the original patch by Marc Titinger.
>>
>> Signed-off-by: Marc Titinger <mtitinger+renesas@baylibre.com>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>> ---
>> drivers/base/power/domain.c | 103 ++++++++++++++++++++++++++++++++++++++++++++
>> include/linux/pm_domain.h | 8 ++++
>> 2 files changed, 111 insertions(+)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 740afa9..368a5b8 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -1895,6 +1895,109 @@ out:
>> return ret ? -EPROBE_DEFER : 0;
>> }
>> EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
>> +
>> +static const struct of_device_id idle_state_match[] = {
>> + { .compatible = "arm,idle-state", },
>
>Do we want ARM-specific compatible values without an #ifdef in drivers/base/?
>
>I know we already have "samsung,power-domain".
>Perhaps that should be protected by #ifdef, too.
>
The arm,idle-state DT binding is re-used here to describe domain idle
states, because that is exactly what we need here. The binding is not
dependent on the ARM architecture, so we won't need a #ifdef around
this. I do agree that using this compatible is not very intuitive.
Thanks,
Lina
^ permalink raw reply
* [PATCH 1/8] PM / Domains: Make genpd state allocation dynamic
From: Lina Iyer @ 2016-10-06 15:40 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAPDyKFpNnJxueNNk9g7KzHPAsJVQ8xeiaw8OHMevBZ3myhd-xA@mail.gmail.com>
On Thu, Oct 06 2016 at 02:37 -0600, Ulf Hansson wrote:
>On 5 October 2016 at 22:31, Lina Iyer <lina.iyer@linaro.org> wrote:
>> Allow PM Domain states to be defined dynamically by the drivers. This
>> removes the limitation on the maximum number of states possible for a
>> domain.
>>
>> Cc: Axel Haslam <ahaslam+renesas@baylibre.com>
>> Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
<...>
>> -#define GENPD_MAX_NUM_STATES 8 /* Number of possible low power states */
>> -
>> enum gpd_status {
>> GPD_STATE_ACTIVE = 0, /* PM domain is active */
>> GPD_STATE_POWER_OFF, /* PM domain is off */
>> @@ -70,7 +68,7 @@ struct generic_pm_domain {
>> void (*detach_dev)(struct generic_pm_domain *domain,
>> struct device *dev);
>> unsigned int flags; /* Bit field of configs for genpd */
>> - struct genpd_power_state states[GENPD_MAX_NUM_STATES];
>> + struct genpd_power_state *states;
>> unsigned int state_count; /* number of states */
>> unsigned int state_idx; /* state that genpd will go to when off */
>>
>> --
>> 2.7.4
>>
>
>In general I like the improvement, but..
>
>This change implies that ->states may very well be NULL. This isn't
>validated by genpd's internal logic when power off/on the domain
>(genpd_power_on|off(), __default_power_down_ok()). You need to fix
>this, somehow.
>
Good point.
>Perhaps the easiest solutions is, when pm_genpd_init() finds that
>->state is NULL, that we allocate a struct genpd_power_state with
>array size of 1 and assign it to ->states. Although, doing this also
>means you need to track that genpd was responsible for the the
>allocation, so it must also free the data from within genpd_remove().
>
>Unless you have other ideas!?
>
I can think of some hacks, but they are uglier than the problem we are
trying to solve. We could drop this patch. Real world situations would
not have more than 8 states and if there is one, we can think about it
then.
Thanks,
Lina
^ permalink raw reply
* [PATCH] ARM: dts: armada-xp-rn2120: add pinmuxing for ethernet
From: Gregory CLEMENT @ 2016-10-06 14:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160929190010.11753-1-u.kleine-koenig@pengutronix.de>
Hi Uwe,
On jeu., sept. 29 2016, Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de> wrote:
> Up to now working ethernet depended on the bootloader to configure the
> pinmuxing. Make it explicit.
>
> As a side effect this change makes ethernet work in barebox.
>
> Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
Applied on mvebu/dt-4.10
Thanks,
Gregory
> ---
> arch/arm/boot/dts/armada-xp-netgear-rn2120.dts | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/arch/arm/boot/dts/armada-xp-netgear-rn2120.dts b/arch/arm/boot/dts/armada-xp-netgear-rn2120.dts
> index 3e930fdbaabc..b6bf5344fbbe 100644
> --- a/arch/arm/boot/dts/armada-xp-netgear-rn2120.dts
> +++ b/arch/arm/boot/dts/armada-xp-netgear-rn2120.dts
> @@ -164,12 +164,18 @@
> };
>
> ethernet at 70000 {
> + pinctrl-0 = <&ge0_rgmii_pins>;
> + pinctrl-names = "default";
> +
> status = "okay";
> phy = <&phy0>;
> phy-mode = "rgmii-id";
> };
>
> ethernet at 74000 {
> + pinctrl-0 = <&ge1_rgmii_pins>;
> + pinctrl-names = "default";
> +
> status = "okay";
> phy = <&phy1>;
> phy-mode = "rgmii-id";
> --
> 2.9.3
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply
* [PATCH] ARM: dts: armada-xp-rn2120: drop wrong compatible for i2c0
From: Gregory CLEMENT @ 2016-10-06 14:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160929185221.23194-1-u.kleine-koenig@pengutronix.de>
Hi Uwe,
On jeu., sept. 29 2016, Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de> wrote:
> The compatible is supposed to be "marvell,mv78230-i2c", "marvell,mv64xxx-i2c",
> as provided by armada-xp.dtsi.
>
> Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
Applied on mvebu/dt-4.10
Thanks,
Gregory
> ---
> arch/arm/boot/dts/armada-xp-netgear-rn2120.dts | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/armada-xp-netgear-rn2120.dts b/arch/arm/boot/dts/armada-xp-netgear-rn2120.dts
> index d19f44c70925..3e930fdbaabc 100644
> --- a/arch/arm/boot/dts/armada-xp-netgear-rn2120.dts
> +++ b/arch/arm/boot/dts/armada-xp-netgear-rn2120.dts
> @@ -97,7 +97,6 @@
> };
>
> i2c at 11000 {
> - compatible = "marvell,mv64xxx-i2c";
> clock-frequency = <400000>;
> status = "okay";
>
> --
> 2.9.3
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply
* [PATCH 2/2] ARM: dts: armada-370-rn104: drop specification of compatible for i2c0
From: Gregory CLEMENT @ 2016-10-06 14:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475173326-24673-2-git-send-email-uwe@kleine-koenig.org>
Hi Uwe,
On jeu., sept. 29 2016, Uwe Kleine-K?nig <uwe@kleine-koenig.org> wrote:
> The compatible string is already provided by armada-370.dtsi.
>
> Signed-off-by: Uwe Kleine-K?nig <uwe@kleine-koenig.org>
Applied on mvebu/dt-4.10
Thanks,
Gregory
> ---
> arch/arm/boot/dts/armada-370-netgear-rn104.dts | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/armada-370-netgear-rn104.dts b/arch/arm/boot/dts/armada-370-netgear-rn104.dts
> index d44a850d879c..14c379699350 100644
> --- a/arch/arm/boot/dts/armada-370-netgear-rn104.dts
> +++ b/arch/arm/boot/dts/armada-370-netgear-rn104.dts
> @@ -126,7 +126,6 @@
> };
>
> i2c at 11000 {
> - compatible = "marvell,mv64xxx-i2c";
> clock-frequency = <100000>;
>
> pinctrl-0 = <&i2c0_pins>;
> --
> 2.8.1
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply
* [PATCH 1/2] ARM: dts: armada-370-rn104: add pinmuxing for i2c0
From: Gregory CLEMENT @ 2016-10-06 14:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475173326-24673-1-git-send-email-uwe@kleine-koenig.org>
Hi Uwe,
On jeu., sept. 29 2016, Uwe Kleine-K?nig <uwe@kleine-koenig.org> wrote:
> Up to now a working i2c bus depended on the bootloader to configure the
> pinmuxing. Make it explicit.
>
> As a side effect this change makes i2c work in barebox.
>
> Signed-off-by: Uwe Kleine-K?nig <uwe@kleine-koenig.org>
Applied on mvebu/dt-4.10
Thanks,
Gregory
> ---
> arch/arm/boot/dts/armada-370-netgear-rn104.dts | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/arm/boot/dts/armada-370-netgear-rn104.dts b/arch/arm/boot/dts/armada-370-netgear-rn104.dts
> index 11565752b9f6..d44a850d879c 100644
> --- a/arch/arm/boot/dts/armada-370-netgear-rn104.dts
> +++ b/arch/arm/boot/dts/armada-370-netgear-rn104.dts
> @@ -128,6 +128,10 @@
> i2c at 11000 {
> compatible = "marvell,mv64xxx-i2c";
> clock-frequency = <100000>;
> +
> + pinctrl-0 = <&i2c0_pins>;
> + pinctrl-names = "default";
> +
> status = "okay";
>
> isl12057: isl12057 at 68 {
> --
> 2.8.1
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply
* [PATCH v2] arm64: dts: marvell: Add definition for the Globalscale Marvell ESPRESSOBin Board
From: Gregory CLEMENT @ 2016-10-06 14:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161006091429.8974-1-romain.perier@free-electrons.com>
Hi Romain,
On jeu., oct. 06 2016, Romain Perier <romain.perier@free-electrons.com> wrote:
> This is a high performance 64 bit dual core low power consuming
> networking computing platform based on the ARMv8 architecture.
> It contains an Armada 3720 running up to 1.2Ghz.
>
> This commit adds a basic definition for this board.
>
> Signed-off-by: Romain Perier <romain.perier@free-electrons.com>
Applied on mvebu/dt64-4.10, I also added that the uart is exported on
the micro USB connector through an FTDI.
Thanks,
Gregory
> ---
>
> Changes in v2:
> - Improved commit message
> - Added informations about connectors
>
> arch/arm64/boot/dts/marvell/Makefile | 1 +
> .../boot/dts/marvell/armada-3720-espressobin.dts | 82 ++++++++++++++++++++++
> 2 files changed, 83 insertions(+)
> create mode 100644 arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
>
> diff --git a/arch/arm64/boot/dts/marvell/Makefile b/arch/arm64/boot/dts/marvell/Makefile
> index 308468d..392eeb6 100644
> --- a/arch/arm64/boot/dts/marvell/Makefile
> +++ b/arch/arm64/boot/dts/marvell/Makefile
> @@ -4,6 +4,7 @@ dtb-$(CONFIG_ARCH_BERLIN) += berlin4ct-stb.dtb
>
> # Mvebu SoC Family
> dtb-$(CONFIG_ARCH_MVEBU) += armada-3720-db.dtb
> +dtb-$(CONFIG_ARCH_MVEBU) += armada-3720-espressobin.dtb
> dtb-$(CONFIG_ARCH_MVEBU) += armada-7040-db.dtb
>
> always := $(dtb-y)
> diff --git a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
> new file mode 100644
> index 0000000..ae005f1
> --- /dev/null
> +++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
> @@ -0,0 +1,82 @@
> +/*
> + * Device Tree file for Globalscale Marvell ESPRESSOBin Board
> + * Copyright (C) 2016 Marvell
> + *
> + * Romain Perier <romain.perier@free-electrons.com>
> + *
> + * This file is dual-licensed: you can use it either under the terms
> + * of the GPL or the X11 license, at your option. Note that this dual
> + * licensing only applies to this file, and not this project as a
> + * whole.
> + *
> + * a) This file 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 file 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.
> + *
> + * Or, alternatively
> + *
> + * b) Permission is hereby granted, free of charge, to any person
> + * obtaining a copy of this software and associated documentation
> + * files (the "Software"), to deal in the Software without
> + * restriction, including without limitation the rights to use
> + * copy, modify, merge, publish, distribute, sublicense, and/or
> + * sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following
> + * conditions:
> + *
> + * The above copyright notice and this permission notice shall be
> + * included in all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED , WITHOUT WARRANTY OF ANY KIND
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
> + * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
> + * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY
> + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +/dts-v1/;
> +
> +#include "armada-372x.dtsi"
> +
> +/ {
> + model = "Globalscale Marvell ESPRESSOBin Board";
> + compatible = "globalscale,espressobin", "marvell,armada3720", "marvell,armada3710";
> +
> + chosen {
> + stdout-path = "serial0:115200n8";
> + };
> +
> + memory {
> + device_type = "memory";
> + reg = <0x00000000 0x00000000 0x00000000 0x20000000>;
> + };
> +};
> +
> +/* J9 */
> +&pcie0 {
> + status = "okay";
> +};
> +
> +/* J6 */
> +&sata {
> + status = "okay";
> +};
> +
> +/* J5 */
> +&uart0 {
> + status = "okay";
> +};
> +
> +/* J7 */
> +&usb3 {
> + status = "okay";
> +};
> --
> 2.9.3
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply
* [PATCH -next] serial: stm32: fix build failure
From: Alexandre Torgue @ 2016-10-06 14:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475746413-18301-1-git-send-email-alexandre.torgue@st.com>
Hi,
On 10/06/2016 11:33 AM, Alexandre TORGUE wrote:
> While building m32r config the build failed with:
> ERROR: stm32-usart.c:(.text+0xdc988): undefined reference to `bad_dma_ops'
>
> To satisfy the dependency CONFIG_SERIAL_STM32 should depend on HAS_DMA.
>
> Signed-off-by: Alexandre TORGUE <alexandre.torgue@st.com>
>
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index c783140..8a261c0 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -1624,6 +1624,7 @@ config SERIAL_SPRD_CONSOLE
>
> config SERIAL_STM32
> tristate "STMicroelectronics STM32 serial port support"
> + depends on HAS_DMA
> select SERIAL_CORE
> depends on ARM || COMPILE_TEST
> help
>
Same patch has been sent in same time. Please abandon this one.
Regards
Alex
^ permalink raw reply
* [PATCH] serial: stm32: Fix comparisons with undefined register
From: Geert Uytterhoeven @ 2016-10-06 13:42 UTC (permalink / raw)
To: linux-arm-kernel
drivers/tty/serial/stm32-usart.c: In function ?stm32_receive_chars?:
drivers/tty/serial/stm32-usart.c:130: warning: comparison is always true due to limited range of data type
drivers/tty/serial/stm32-usart.c: In function ?stm32_tx_dma_complete?:
drivers/tty/serial/stm32-usart.c:177: warning: comparison is always false due to limited range of data type
stm32_usart_offsets.icr is u8, while UNDEF_REG = ~0 is int, and thus
0xffffffff.
As all registers in stm32_usart_offsets are u8, change the definition of
UNDEF_REG to 0xff to fix this.
Fixes: ada8618ff3bfe183 ("serial: stm32: adding support for stm32f7")
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
Untested due to lack of hardware.
---
drivers/tty/serial/stm32-usart.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/stm32-usart.h b/drivers/tty/serial/stm32-usart.h
index 41d97492310271db..cd97ceb76e4ffe6f 100644
--- a/drivers/tty/serial/stm32-usart.h
+++ b/drivers/tty/serial/stm32-usart.h
@@ -31,7 +31,7 @@ struct stm32_usart_info {
struct stm32_usart_config cfg;
};
-#define UNDEF_REG ~0
+#define UNDEF_REG 0xff
/* Register offsets */
struct stm32_usart_info stm32f4_info = {
--
1.9.1
^ permalink raw reply related
* [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06
From: John Garry @ 2016-10-06 13:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475713112.3784.183.camel@kernel.crashing.org>
On 06/10/2016 01:18, Benjamin Herrenschmidt wrote:
> On Tue, 2016-10-04 at 13:02 +0100, John Garry wrote:
>> Right, so I think Zhichang can make the necessary generic changes to
>> 8250 OF driver to support IO port as well as MMIO-based.
>>
>> However an LPC-based earlycon driver is still required.
>>
>> A note on hip07-based D05 (for those unaware): this does not use
>> LPC-based uart. It uses PL011. The hardware guys have managed some
>> trickery where they loopback the serial line around the BMC/CPLD. But we
>> still need it for hip06 D03 and any other boards which want to use LPC
>> bus for uart.
>>
>> A question on SBSA: does it propose how to provide serial via BMC for SOL?
>
> Probably another reason to keep 8250 as a legal option ... The (very
> popular) Aspeed BMCs tend to do this via a 8250-looking virtual UART on
> LPC.
>
> Cheers,
> Ben,
I think we're talking about the same thing for our LPC-based UART.
John
>
>
> .
>
^ permalink raw reply
* [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver
From: Jassi Brar @ 2016-10-06 13:10 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475758897.4102.31.camel@mtksdaap41>
On 6 October 2016 at 18:31, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
> Back to our original statement, we need to flush all tasks to queue
> in GCE HW; i.e. we need to use mbox_client_txdone after
> mbox_send_message, or send tx_done once mailbox controller receive
> message (task). However, we still need a way to notice done tasks to
> clients. Currently, we don't have a good way to call callback in mailbox
> framework. Therefore, CMDQ driver has its owner callback functions.
>
mbox_client_txdone() is called by the client driver when only it knows
the messages has been transmitted (i.e your submitted tasks are done).
Obviously the client driver should do any callbacks to its users
upstream.
^ permalink raw reply
* [PATCH] exynos-drm: Fix error messages to print flags and size
From: Tobias Jakobi @ 2016-10-06 13:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1470850236-9101-1-git-send-email-shuahkh@osg.samsung.com>
Hello,
I think this patch was never picked up. So just a short 'ping' from my side.
With best wishes,
Tobias
Shuah Khan wrote:
> Fix exynos_drm_gem_create() error messages to include flags and size when
> flags and size are invalid.
>
> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> ---
> drivers/gpu/drm/exynos/exynos_drm_gem.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> index cdf9f1a..4c4cb0e 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> @@ -231,12 +231,12 @@ struct exynos_drm_gem *exynos_drm_gem_create(struct drm_device *dev,
> int ret;
>
> if (flags & ~(EXYNOS_BO_MASK)) {
> - DRM_ERROR("invalid flags.\n");
> + DRM_ERROR("invalid GEM buffer flags: %u\n", flags);
> return ERR_PTR(-EINVAL);
> }
>
> if (!size) {
> - DRM_ERROR("invalid size.\n");
> + DRM_ERROR("invalid GEM buffer size: %lu\n", size);
> return ERR_PTR(-EINVAL);
> }
>
>
^ permalink raw reply
* [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver
From: Horng-Shyang Liao @ 2016-10-06 13:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAJe_ZheeEe0xEj733pxr4q=-YzxKHgRM4MMPDJNHa9uRC9Q2zA@mail.gmail.com>
On Wed, 2016-10-05 at 20:13 +0530, Jassi Brar wrote:
> On 5 October 2016 at 18:01, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
> > On Wed, 2016-10-05 at 09:07 +0530, Jassi Brar wrote:
> >> On 5 October 2016 at 08:24, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
> >> > On Fri, 2016-09-30 at 17:47 +0800, Horng-Shyang Liao wrote:
> >> >> On Fri, 2016-09-30 at 17:11 +0800, CK Hu wrote:
> >>
> >> >
> >> > After I trace mailbox driver, I realize that CMDQ driver cannot use
> >> > tx_done.
> >> >
> >> > CMDQ clients will flush many tasks into CMDQ driver, and then CMDQ
> >> > driver will apply these tasks into GCE HW "immediately". These tasks,
> >> > which are queued in GCE HW, may not execute immediately since they
> >> > may need to wait event(s), e.g. vsync.
> >> >
> >> > However, in mailbox driver, mailbox uses a software buffer to queue
> >> > sent messages. It only sends next message until previous message is
> >> > done. This cannot fulfill CMDQ's requirement.
> >> >
> >> I understand
> >> a) GCE HW can internally queue many tasks in some 'FIFO'
> >> b) Execution of some task may have to wait until some external event
> >> occurs (like vsync)
> >> c) GCE does not generate irq/flag for each task executed (?)
> >>
> >> If so, may be your tx_done should return 'true' so long as the GCE HW
> >> can accept tasks in its 'FIFO'. For mailbox api, any task that is
> >> queued on GCE, is assumed to be transmitted.
> >>
> >> > Quote some code from mailbox driver. Please notice "active_req" part.
> >> >
> >> > static void msg_submit(struct mbox_chan *chan)
> >> > {
> >> > ...
> >> > if (!chan->msg_count || chan->active_req)
> >> > goto exit;
> >> > ...
> >> > err = chan->mbox->ops->send_data(chan, data);
> >> > if (!err) {
> >> > chan->active_req = data;
> >> > chan->msg_count--;
> >> > }
> >> > ...
> >> > }
> >> >
> >> > static void tx_tick(struct mbox_chan *chan, int r)
> >> > {
> >> > ...
> >> > spin_lock_irqsave(&chan->lock, flags);
> >> > mssg = chan->active_req;
> >> > chan->active_req = NULL;
> >> > spin_unlock_irqrestore(&chan->lock, flags);
> >> > ...
> >> > }
> >> >
> >> > Current workable CMDQ driver uses mbox_client_txdone() to prevent
> >> > this issue, and then uses self callback functions to handle done tasks.
> >> >
> >> > int cmdq_task_flush_async(struct cmdq_client *client, struct cmdq_task
> >> > *task, cmdq_async_flush_cb cb, void *data)
> >> > {
> >> > ...
> >> > mbox_send_message(client->chan, task);
> >> > /* We can send next task immediately, so just call txdone. */
> >> > mbox_client_txdone(client->chan, 0);
> >> > ...
> >> > }
> >> >
> >> > Another solution is to use rx_callback; i.e. CMDQ mailbox controller
> >> > call mbox_chan_received_data() when CMDQ task is done. But, this may
> >> > violate the design of mailbox. What do you think?
> >> >
> >> If my point (c) above does not hold, maybe look at implementing
> >> tx_done() callback and submit next task from the callback of last
> >> done.
> >
> >
> > Hi Jassi,
> >
> > For point (c), GCE irq means 1~n tasks done or
> > 0~n tasks done + 1 task error.
> > In irq, we can know which tasks are done by register and GCE pc.
> >
> > As I mentioned before, we cannot submit next task after previous task
> > call tx_done. We need to submit multiple tasks to GCE HW immediately
> > and queue them in GCE HW.
>
> > Let me explain this requirement by mouse
> > cursor example. User may move mouse quickly between two vsync, so DRM
> > may update display registers frequently. For CMDQ, that means many tasks
> > are flushed into CMDQ driver, and CMDQ driver needs to process all of
> > them in next vblank. Therefore, we cannot block any CMDQ task in SW
> > buffer.
> >
> We are interested only in the current position of cursor and not its
> trail. Also the current position should be updated at next vsync (and
> not the one after it).
> Going by this example, if the GCE HW can take in 'N' tasks at a time,
> then the N+1th submission should shift out (drop) the 1st task queued.
> So that at any time GCE HW has only the latest N tasks. Right?
>
> If yes, maybe you don't need to care about tx-done and simply keep
> shoving tasks as you generate them.
>
> If no, maybe your client driver need to emulate such a circular
> buffer where oldest task is overwritten by newest submission. And you
> submit the circular buffer (most relevant tasks) at one go to the GCE
> HW.
Hi Jassi,
CMDQ driver doesn't know the task type, so CMDQ cannot decide which
tasks can be dropped. So, I think the answer is "no".
Client driver is also hard to emulate a circular buffer where oldest
task is overwritten by newest submission. Let me illustrate with DRM
client (display driver). For DRM, the only way to know the latest
mouse cursor task before vsync is vsync interrupt. However, if we
keep latest mouse cursor task and start to send it to CMDQ after
vsync, the task may be executed outside the vblank and generate garbage
on screen in this case. (since we allow nested interrupt) We can wait
one more vsync to prevent garbage, but we will lose mouse cursor
performance. Therefore, our current solution is to flush all DRM tasks
into CMDQ, and queue all tasks in GCE HW. When GCE get vsync event, it
can finish all tasks within vblank to fulfill DRM's requirement.
Back to our original statement, we need to flush all tasks to queue
in GCE HW; i.e. we need to use mbox_client_txdone after
mbox_send_message, or send tx_done once mailbox controller receive
message (task). However, we still need a way to notice done tasks to
clients. Currently, we don't have a good way to call callback in mailbox
framework. Therefore, CMDQ driver has its owner callback functions.
Thanks,
HS
^ permalink raw reply
* [PATCH v2] Adding missing features of Coresight PTM components
From: Muhammad Abdul WAHAB @ 2016-10-06 12:20 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CANLsYkxepzF1VuU6uDSrMEmd6F5ET_yyM73eBSvypaTJCu4eyA@mail.gmail.com>
Hi Mathieu,
> Please remove the warning message as well - there is no point having
> it there because:
>
> 1) From sysFS users are supposed to know what they're doing.
> 2) If we start warning users on all the things that can go wrong the
> code will become unbelievably cluttered.
>
> Thanks,
> Mathieu
A third version of the patch was sent to you. Thank you for explanation
about warnings. I had forgotten two defines that I added also in the patch.
Thanks,
M.Abdul WAHAB
^ permalink raw reply
* [PATCH v3] Adding missing features of Coresight PTM components
From: Muhammad Abdul WAHAB @ 2016-10-06 12:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <f029bcee-696f-3519-b0f1-4cdcf24ad3f8@centralesupelec.fr>
In the current driver for Coresight components, two features of PTM
components are missing:
1. Branch Broadcasting (present also in ETM but called Branch Output)
2. Return Stack (only present in PTM v1.0 and PTMv1.1)
These features can be added simply to the code using `mode` field of
`etm_config` struct.
1. **Branch Broadcast** : The branch broadcast feature is present in ETM
components as well and is called Branch output. It allows to retrieve
addresses for direct branch addresses alongside the indirect branch
addresses. For example, it could be useful in cases when tracing without
source code.
2. **Return Stack** : The return stack option allows to retrieve the return
addresses of function calls. It can be useful to avoid CRA
(Code Reuse Attacks) by keeping a shadowstack.
Signed-off-by: Muhammad Abdul Wahab <muhammadabdul.wahab@centralesupelec.fr>
---
changes in v3 :
- deleted warning message
- bit field definition of Branch Broadcast and Return Stack
drivers/hwtracing/coresight/coresight-etm.h | 5 +++++
drivers/hwtracing/coresight/coresight-etm3x-sysfs.c | 10 ++++++++++
2 files changed, 15 insertions(+)
diff --git a/drivers/hwtracing/coresight/coresight-etm.h
b/drivers/hwtracing/coresight/coresight-etm.h
index 4a18ee4..ad063d7 100644
--- a/drivers/hwtracing/coresight/coresight-etm.h
+++ b/drivers/hwtracing/coresight/coresight-etm.h
@@ -89,11 +89,13 @@
/* ETMCR - 0x00 */
#define ETMCR_PWD_DWN BIT(0)
#define ETMCR_STALL_MODE BIT(7)
+#define ETMCR_BRANCH_BROADCAST BIT(8)
#define ETMCR_ETM_PRG BIT(10)
#define ETMCR_ETM_EN BIT(11)
#define ETMCR_CYC_ACC BIT(12)
#define ETMCR_CTXID_SIZE (BIT(14)|BIT(15))
#define ETMCR_TIMESTAMP_EN BIT(28)
+#define ETMCR_RETURN_STACK BIT(29)
/* ETMCCR - 0x04 */
#define ETMCCR_FIFOFULL BIT(23)
/* ETMPDCR - 0x310 */
@@ -110,8 +112,11 @@
#define ETM_MODE_STALL BIT(2)
#define ETM_MODE_TIMESTAMP BIT(3)
#define ETM_MODE_CTXID BIT(4)
+#define ETM_MODE_BBROAD BIT(5)
+#define ETM_MODE_RET_STACK BIT(6)
#define ETM_MODE_ALL (ETM_MODE_EXCLUDE | ETM_MODE_CYCACC | \
ETM_MODE_STALL | ETM_MODE_TIMESTAMP | \
+ ETM_MODE_BBROAD | ETM_MODE_RET_STACK | \
ETM_MODE_CTXID | ETM_MODE_EXCL_KERN | \
ETM_MODE_EXCL_USER)
diff --git a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
index 5ea0909..a76009a 100644
--- a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
@@ -164,6 +164,16 @@ static ssize_t mode_store(struct device *dev,
else
config->ctrl &= ~ETMCR_CTXID_SIZE;
+ if (config->mode & ETM_MODE_BBROAD)
+ config->ctrl |= ETMCR_BRANCH_BROADCAST;
+ else
+ config->ctrl &= ~ETMCR_BRANCH_BROADCAST;
+
+ if (config->mode & ETM_MODE_RET_STACK)
+ config->ctrl |= ETMCR_RETURN_STACK_EN;
+ else
+ config->ctrl &= ~ETMCR_RETURN_STACK_EN;
+
if (config->mode & (ETM_MODE_EXCL_KERN | ETM_MODE_EXCL_USER))
etm_config_trace_mode(config);
--
1.9.1
^ permalink raw reply related
* [PATCH v7 0/7] arm/arm64: vgic: Implement API for vGICv3 live migration
From: Christoffer Dall @ 2016-10-06 12:14 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CALicx6uFBF=J5J4L6sVspqMx-sjGCjoAgONHAe5Q4msonfVYEA@mail.gmail.com>
Hi Vijay,
On Wed, Oct 5, 2016 at 4:33 PM, Vijay Kilari <vijay.kilari@gmail.com> wrote:
> Hi Marc, Christoffer,
>
> Do you have any review comments on this patch set?.
>
Since we missed the 4.9 merge window there is no urgency just yet, and
I'm tied up with other things for a while, and Marc is away this week
on holiday. I'm sure he'll have a look when he's back and I'll follow
up later.
Thanks,
-Christoffer
^ permalink raw reply
* [PATCH 4/8] PM / Domains: Add fwnode provider to genpd states
From: Ulf Hansson @ 2016-10-06 12:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475699519-109623-5-git-send-email-lina.iyer@linaro.org>
On 5 October 2016 at 22:31, Lina Iyer <lina.iyer@linaro.org> wrote:
> Save the fwnode for the genpd state in the state node. PM Domain clients
> may use the fwnode to read in the rest of the properties for the domain
> state.
What is the "rest"? I assume you mean the non PM domain generic parts,
but perhaps you could elaborate a bit on that?
>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> ---
> drivers/base/power/domain.c | 1 +
> include/linux/pm_domain.h | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 368a5b8..52fcdb2 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1937,6 +1937,7 @@ static int read_genpd_state(struct genpd_power_state *genpd_state,
> latency = entry_latency + exit_latency;
> genpd_state->power_on_latency_ns = 1000 * latency;
> genpd_state->power_off_latency_ns = 1000 * entry_latency;
> + genpd_state->provider = &state_node->fwnode;
>
> return 0;
> }
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 4c9152d..eacfa71 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -39,6 +39,7 @@ struct genpd_power_state {
> s64 power_off_latency_ns;
> s64 power_on_latency_ns;
> s64 residency_ns;
> + struct fwnode_handle *provider;
I don't think this is a provider, but just a fwnode_handle to a
domain-idle-state. Therefore, I would suggest you to rename it to
"fwnode" instead.
> };
>
> struct generic_pm_domain {
> --
> 2.7.4
>
Kind regards
Uffe
^ permalink raw reply
* [PATCH 8/8] PM / doc: Update device documentation for devices in IRQ safe PM domains
From: Ulf Hansson @ 2016-10-06 11:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475699519-109623-9-git-send-email-lina.iyer@linaro.org>
On 5 October 2016 at 22:31, Lina Iyer <lina.iyer@linaro.org> wrote:
> Update documentation to reflect the changes made to support IRQ safe PM
> domains.
>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> ---
> Documentation/power/devices.txt | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/power/devices.txt b/Documentation/power/devices.txt
> index 8ba6625..a622136 100644
> --- a/Documentation/power/devices.txt
> +++ b/Documentation/power/devices.txt
> @@ -607,7 +607,17 @@ individually. Instead, a set of devices sharing a power resource can be put
> into a low-power state together at the same time by turning off the shared
> power resource. Of course, they also need to be put into the full-power state
> together, by turning the shared power resource on. A set of devices with this
> -property is often referred to as a power domain.
> +property is often referred to as a power domain. A power domain may also be
> +nested inside another power domain.
> +
> +Devices, by default, operate in process context. If a device can operate in
> +IRQ safe context that has to be explicitly indicated by setting the irq_safe
> +boolean inside struct generic_pm_domain to be true. Power domains by default,
This doesn't make much sense.
First, I think you should mention that this is about which context the
device's corresponding runtime PM callbacks are allowed to execute in.
Second, it's not clear *why* you must set the irq_safe boolean inside
struct generic_pm_domain to true, for IRQ-safe devices. You probably
shouldn't state this at all.
Perhaps a better description is to elaborate on the constraints an
IRQ-safe device holds. Then one could explain a bit about how IRQ safe
(genpd) PM domains can be used for these devices (something like
below).
> +operate in process context but could have devices that are IRQ safe. Such
> +power domains cannot be powered on/off during runtime PM. On the other hand,
> +IRQ safe PM domains that have IRQ safe devices may be powered off when all
> +the devices are in idle. An IRQ safe domain may only be attached as a
> +subdomain to another IRQ safe domain.
>
> Support for power domains is provided through the pm_domain field of struct
> device. This field is a pointer to an object of type struct dev_pm_domain,
> --
> 2.7.4
>
Kind regards
Uffe
^ permalink raw reply
* [PATCH v5 2/5] drm/bridge: Add RGB to VGA bridge support
From: Archit Taneja @ 2016-10-06 11:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161006072106.GA4684@lukather>
On 10/06/2016 12:51 PM, Maxime Ripard wrote:
> Hi Archit,
>
> On Mon, Oct 03, 2016 at 04:40:57PM +0530, Archit Taneja wrote:
>> Hi Maxime,
>>
>> On 09/30/2016 08:07 PM, Maxime Ripard wrote:
>>> Some boards have an entirely passive RGB to VGA bridge, based on either
>>> DACs or resistor ladders.
>>>
>>> Those might or might not have an i2c bus routed to the VGA connector in
>>> order to access the screen EDIDs.
>>>
>>> Add a bridge that doesn't do anything but expose the modes available on the
>>> screen, either based on the EDIDs if available, or based on the XGA
>>> standards.
>>>
>>> Acked-by: Rob Herring <robh@kernel.org>
>>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>>> ---
>>> .../bindings/display/bridge/rgb-to-vga-bridge.txt | 48 +++++
>>> drivers/gpu/drm/bridge/Kconfig | 7 +
>>> drivers/gpu/drm/bridge/Makefile | 1 +
>>> drivers/gpu/drm/bridge/rgb-to-vga.c | 229 +++++++++++++++++++++
>>> 4 files changed, 285 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.txt
>>> create mode 100644 drivers/gpu/drm/bridge/rgb-to-vga.c
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.txt b/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.txt
>>> new file mode 100644
>>> index 000000000000..a8375bc1f9cb
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/display/bridge/rgb-to-vga-bridge.txt
>>> @@ -0,0 +1,48 @@
>>> +Dumb RGB to VGA bridge
>>> +----------------------
>>> +
>>> +This binding is aimed for dumb RGB to VGA bridges that do not require
>>> +any configuration.
>>> +
>>> +Required properties:
>>> +
>>> +- compatible: Must be "rgb-to-vga-bridge"
>>
>> I'd talked to Laurent on IRC if he's okay with this. And I guess you to
>> had discussed it during XDC too. He's suggested that it'd be better to
>> have the compatible string as "simple-vga-dac".
>
> I just wished this bikeshedding had taken place publicly and be
> actually part of that discussion, but yeah, ok.
Sorry about that. I'd pinged him for an Ack, the discussion went
more than that :)
>
>> Some of the reasons behind having this:
>>
>> - We don't need to specify "rgb" in the compatible string since most
>> simple VGA DACs can only work with an RGB input.
>
> Ok.
>
>> - Also, with "dac" specified in the string, we don't need to
>> specifically mention "bridge" in the string. Also, bridge is a drm
>> specific term.
>>
>> - "simple" is considered because it's an unconfigurable bridge, and it
>> might be misleading for other VGA DACs to not use "vga-dac".
>
> All those "simple" bindings are just the biggest lie we ever
> told. It's simple when you introduce it, and then grows into something
> much more complicated than a non-simple implementation.
"simple" here is supposed to mean that it's an unconfigurable RGB to
VGA DAC. This isn't supposed to follow the simple-panel model, where
you add the "simple-panel" string in the compatible node, along with
you chip specific compatible string.
In other words, this driver shouldn't be touched again in the future :)
If someone wants to write a RGB to VGA driver which is even
slightly configurable, they'll need to write a new bridge driver.
Thanks,
Archit
>
>> What do you think about this? If you think it's good, would it be
>> possible for you to change this? I guess it's okay for the rest of
>> the patch to stay the same.
>
> I'll update and respin the serie.
>
> Thanks,
> Maxime
>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* [PATCH 7/8] PM / Domains: Support IRQ safe PM domains
From: Ulf Hansson @ 2016-10-06 11:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475699519-109623-8-git-send-email-lina.iyer@linaro.org>
On 5 October 2016 at 22:31, Lina Iyer <lina.iyer@linaro.org> wrote:
> Generic Power Domains currently support turning on/off only in process
> context. This prevents the usage of PM domains for domains that could be
> powered on/off in a context where IRQs are disabled. Many such domains
> exist today and do not get powered off, when the IRQ safe devices in
> that domain are powered off, because of this limitation.
>
> However, not all domains can operate in IRQ safe contexts. Genpd
> therefore, has to support both cases where the domain may or may not
> operate in IRQ safe contexts. Configuring genpd to use an appropriate
> lock for that domain, would allow domains that have IRQ safe devices to
> runtime suspend and resume, in atomic context.
>
> To achieve domain specific locking, set the domain's ->flag to
> GENPD_FLAG_IRQ_SAFE while defining the domain. This indicates that genpd
> should use a spinlock instead of a mutex for locking the domain. Locking
> is abstracted through genpd_lock() and genpd_unlock() functions that use
> the flag to determine the appropriate lock to be used for that domain.
>
> Domains that have lower latency to suspend and resume and can operate
> with IRQs disabled may now be able to save power, when the component
> devices and sub-domains are idle at runtime.
>
> The restriction this imposes on the domain hierarchy is that non-IRQ
> safe domains may not have IRQ-safe subdomains, but IRQ safe domains may
> have IRQ safe and non-IRQ safe subdomains and devices.
>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> ---
> drivers/base/power/domain.c | 107 +++++++++++++++++++++++++++++++++++++++-----
> include/linux/pm_domain.h | 10 ++++-
> 2 files changed, 106 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 82e6a33..77e92c2 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -74,11 +74,61 @@ static const struct genpd_lock_fns genpd_mtx_fns = {
> .unlock = genpd_unlock_mtx,
> };
>
> +static void genpd_lock_spin(struct generic_pm_domain *genpd)
> + __acquires(&genpd->slock)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&genpd->slock, flags);
> + genpd->lock_flags = flags;
> +}
> +
> +static void genpd_lock_nested_spin(struct generic_pm_domain *genpd,
> + int depth)
> + __acquires(&genpd->slock)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave_nested(&genpd->slock, flags, depth);
> + genpd->lock_flags = flags;
> +}
> +
> +static int genpd_lock_interruptible_spin(struct generic_pm_domain *genpd)
> + __acquires(&genpd->slock)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&genpd->slock, flags);
> + genpd->lock_flags = flags;
> + return 0;
> +}
> +
> +static void genpd_unlock_spin(struct generic_pm_domain *genpd)
> + __releases(&genpd->slock)
> +{
> + spin_unlock_irqrestore(&genpd->slock, genpd->lock_flags);
> +}
> +
> +static const struct genpd_lock_fns genpd_spin_fns = {
> + .lock = genpd_lock_spin,
> + .lock_nested = genpd_lock_nested_spin,
> + .lock_interruptible = genpd_lock_interruptible_spin,
> + .unlock = genpd_unlock_spin,
> +};
> +
> #define genpd_lock(p) p->lock_fns->lock(p)
> #define genpd_lock_nested(p, d) p->lock_fns->lock_nested(p, d)
> #define genpd_lock_interruptible(p) p->lock_fns->lock_interruptible(p)
> #define genpd_unlock(p) p->lock_fns->unlock(p)
>
> +#define genpd_is_irq_safe(genpd) (genpd->flags & GENPD_FLAG_IRQ_SAFE)
> +
> +static inline bool irq_safe_dev_in_no_sleep_domain(struct device *dev,
> + struct generic_pm_domain *genpd)
> +{
> + return pm_runtime_is_irq_safe(dev) && !genpd_is_irq_safe(genpd);
> +}
> +
> /*
> * Get the generic PM domain for a particular struct device.
> * This validates the struct device pointer, the PM domain pointer,
> @@ -343,7 +393,12 @@ static int genpd_poweroff(struct generic_pm_domain *genpd, bool is_async)
> if (stat > PM_QOS_FLAGS_NONE)
> return -EBUSY;
>
> - if (!pm_runtime_suspended(pdd->dev) || pdd->dev->power.irq_safe)
> + /*
> + * Do not allow PM domain to be powered off, when an IRQ safe
> + * device is part of a non-IRQ safe domain.
> + */
> + if (!pm_runtime_suspended(pdd->dev) ||
> + irq_safe_dev_in_no_sleep_domain(pdd->dev, genpd))
> not_suspended++;
> }
>
> @@ -506,10 +561,10 @@ static int genpd_runtime_suspend(struct device *dev)
> }
>
> /*
> - * If power.irq_safe is set, this routine will be run with interrupts
> - * off, so it can't use mutexes.
> + * If power.irq_safe is set, this routine may be run with
> + * IRQs disabled, so suspend only if the PM domain also is irq_safe.
> */
> - if (dev->power.irq_safe)
> + if (irq_safe_dev_in_no_sleep_domain(dev, genpd))
> return 0;
>
> genpd_lock(genpd);
> @@ -543,8 +598,11 @@ static int genpd_runtime_resume(struct device *dev)
> if (IS_ERR(genpd))
> return -EINVAL;
>
> - /* If power.irq_safe, the PM domain is never powered off. */
> - if (dev->power.irq_safe) {
> + /*
> + * As we don't power off a non IRQ safe domain, which holds
> + * an IRQ safe device, we don't need to restore power to it.
> + */
> + if (irq_safe_dev_in_no_sleep_domain(dev, genpd)) {
> timed = false;
> goto out;
> }
> @@ -586,7 +644,8 @@ static int genpd_runtime_resume(struct device *dev)
> err_stop:
> genpd_stop_dev(genpd, dev);
> err_poweroff:
> - if (!dev->power.irq_safe) {
> + if (!dev->power.irq_safe ||
> + (dev->power.irq_safe && genpd_is_irq_safe(genpd))) {
Please take the opportunity to convert into use
pm_runtime_is_irq_safe(), in favour of checking "dev->power.irq_safe".
> genpd_lock(genpd);
> genpd_poweroff(genpd, 0);
> genpd_unlock(genpd);
> @@ -1111,6 +1170,11 @@ static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
> if (IS_ERR(gpd_data))
> return PTR_ERR(gpd_data);
>
> + /* Check if we are adding an IRQ safe device to non-IRQ safe domain */
> + if (irq_safe_dev_in_no_sleep_domain(dev, genpd))
> + dev_warn_once(dev, "PM domain %s will not be powered off\n",
> + genpd->name);
> +
It may turn out that a subsystem/driver decides to enable
pm_runtime_irq_safe() for the device at a later point, while probing.
Due to that, this warning is not going to be printed.
If we want this warning printed, I think we should strive towards a
more consistent behaviour. Perhaps we should move this print inside
irq_safe_dev_in_no_sleep_domain()?
[...]
One more thing that popped up in my mind. We currently also have the
"pm_domain_always_on_gov". If we checked for such configuration and
before taking the lock for the genpd during runtime suspend, we would
be able to allow IRQ safe device to be suspended when they reside in
these types of always-on domains, don't you think?
Anyway, you don't need to do that change as part of $subject patch,
but perhaps we might want to extend the support for irq safe devices
later on?
Besides the minor nitpicks, this looks good to me!
Kind regards
Uffe
^ permalink raw reply
* [PATCH v9 17/19] drm/virtio: kconfig: Fix recursive dependency issue.
From: Emil Velikov @ 2016-10-06 11:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161006104807.GB436@griffinp-ThinkPad-X1-Carbon-2nd>
Hi Peter,
On 6 October 2016 at 11:48, Peter Griffin <peter.griffin@linaro.org> wrote:
> Hi Emil,
>
> On Wed, 21 Sep 2016, Emil Velikov wrote:
>
>> On 20 September 2016 at 09:32, Peter Griffin <peter.griffin@linaro.org> wrote:
>> > Hi Emil,
>> >
>> > On Tue, 20 Sep 2016, Emil Velikov wrote:
>> >
>> >> On 5 September 2016 at 14:16, Peter Griffin <peter.griffin@linaro.org> wrote:
>> >> > ST_SLIM_REMOTEPROC must select REMOTEPROC, which exposes the following
>> >> > recursive dependency.
>> >
>> >
>> >> >
>> >> From a humble skim through remoteproc, drm and a few other subsystems
>> >> I think the above is wrong. All the drivers (outside of remoteproc),
>> >> that I've seen, depend on the core component, they don't select it.
>> >
>> > I will let Bjorn comment on the remoteproc subsystem Kconfig design, and
>> > why it is like it is.
>> >
>> > For this particular SLIM_RPROC I have added it to Kconfig in keeping with all
>> > the other drivers in the remoteproc subsystem which has exposed this recursive
>> > dependency issue.
>> >
>> > For this particular kconfig symbol a quick grep reveals more drivers in
>> > the kernel using 'select', than 'depend on'
>> >
>> > git grep "select VIRTIO" | wc -l
>> > 14
>> >
>> > git grep "depends on VIRTIO" | wc -l
>> > 10
>> >
>> Might be worth taking a closer look into these at some point.
>
> VIRTIO has no dependencies, and is a non visible symbol. Its Kconfig help also
> mentions that drivers should select it.
>
This is a (un)fortunate detail cannot/should not overrule the other
arguments I've mentioned, should it ?
>>
>> >
>> >> Furthermore most places explicitly hide the drivers from the menu if
>> >> the core component isn't enabled.
>> >
>> > Remoteproc subsystem takes a different approach, the core code is only enabled
>> > if a driver which relies on it is enabled. This IMHO makes sense, as
>> > remoteproc is not widely used (only a few particular ARM SoC's).
>> >
>> > It is true that for subsystems which rely on the core component being
>> > explicitly enabled, they often tend to hide drivers which depend on it
>> > from the menu unless it is. This also makes sense.
>> >
>> >>
>> >> Is there something that requires such a different/unusual behaviour in
>> >> remoteproc ?
>> >>
>> >
>> > I'm not sure it is that unusual...looking at config USB, it selects USB_COMMON in
>> > mfd subsystem, client drivers select MFD_CORE.
>> >
>> On the USB case I'm not sure what the reasoning behind the USB vs
>> USB_COMMON split. In seems that one could just fold them, but that's
>> another topic. On the MFD side... it follows the select {,mis,ab}use.
>> With one (the only one?) MFD driver not using/selecting MFD_CORE doing
>> it's own version of mfd_add_devices... which could be reworked,
>> possibly.
>
> Much like selecting VIRTIO in this patch, MFD_CORE is a non visible symbol
> with no dependencies so it matches the documentation Jani referenced.
>
> I personally think the approach taken makes sense, as why would you want to have
> a CONFIG_MFD_CORE=y symbol being enabled unless you actually have a MFD device
> which uses it also enabled in your kernel?
>
> It seems to me a good solution to make the client drivers select the core
> component so that it only gets enabled if at least one driver requires it.
> This avoids having useless core code which will never be used compiled into the
> kernel and in the end a smaller kernel size for a given kernel configuration (better
> cache use etc etc).
>
>> > I've added Arnd to this thread, as I've seen lots of Kconfig patches from him
>> > recently, maybe he has some thoughts on whether this is the correct fix or not?
>> >
>> Ack. Fwiw, I believe that the reasoning put by Jani is perfeclty
>> reasonable, but it'll be great to hear others as well.
>
> Yes me to. However I don't think anything in this patch is at odds with the
> documentation Jani has referenced.
>
It case it's not clear, let me elaborate:
Yes, using depend might not be the most user-friendly thing to do and
in the long term we might want to move to select.
Yes, I agree with the argument about symbol visibility but that is not
the only contributing factor.
If one wants to pick on specific users which opt for $driver select
$core they must do the same for $driver depends on $core. Check the
number 'in favour" of each case and draw their conclusions ;-)
In particular: both MFD_CORE and USB_COMMON, are _optional_ as only
some drivers depends on them. Thus giving them as an example is
wrong/irrelevant, I'm afraid.
Thanks
Emil
^ 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