* Re: [PATCH v8 6/6] PM / devfreq: Use PM QoS for sysfs min/max_freq
From: Leonard Crestez @ 2019-09-25 22:11 UTC (permalink / raw)
To: Chanwoo Choi, Matthias Kaehlcke
Cc: Artur Świgoń, Abel Vesa, Saravana Kannan,
linux-pm@vger.kernel.org, Viresh Kumar, dl-linux-imx,
Krzysztof Kozlowski, Lukasz Luba, Kyungmin Park, MyungJoo Ham,
Alexandre Bailon, Georgi Djakov,
linux-arm-kernel@lists.infradead.org, Jacky Bai
In-Reply-To: <c521989f-51b6-84eb-b4f1-c4469494345e@samsung.com>
On 25.09.2019 05:36, Chanwoo Choi wrote:
> On 19. 9. 24. 오후 7:11, Leonard Crestez wrote:
>> Switch the handling of min_freq and max_freq from sysfs to use the
>> dev_pm_qos_request interface.
>>
>> Since PM QoS handles frequencies as kHz this change reduces the
>> precision of min_freq and max_freq. This shouldn't introduce problems
>> because frequencies which are not an integer number of kHz are likely
>> not an integer number of Hz either.
>>
>> Try to ensure compatibility by rounding min values down and rounding
>> max values up.
>>
>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>> ---
>> drivers/devfreq/devfreq.c | 46 ++++++++++++++++++++++++---------------
>> include/linux/devfreq.h | 9 ++++----
>> 2 files changed, 33 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 784f3e40536a..8bb7efd821ab 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -137,14 +137,10 @@ static void get_freq_range(struct devfreq *devfreq,
>> qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent,
>> DEV_PM_QOS_MIN_FREQUENCY);
>> *min_freq = max(*min_freq, HZ_PER_KHZ * qos_min_freq);
>> *max_freq = min(*max_freq, HZ_PER_KHZ * qos_max_freq);
>>
>> - /* constraints from sysfs */
>> - *min_freq = max(*min_freq, devfreq->min_freq);
>> - *max_freq = min(*max_freq, devfreq->max_freq);
>> -
>> /* constraints from OPP interface */
>> *min_freq = max(*min_freq, devfreq->scaling_min_freq);
>> /* scaling_max_freq can be zero on error */
>> if (devfreq->scaling_max_freq)
>> *max_freq = min(*max_freq, devfreq->scaling_max_freq);
>> @@ -679,10 +675,12 @@ static void devfreq_dev_release(struct device *dev)
>> DEV_PM_QOS_MIN_FREQUENCY);
>>
>> if (devfreq->profile->exit)
>> devfreq->profile->exit(devfreq->dev.parent);
>>
>> + dev_pm_qos_remove_request(&devfreq->user_max_freq_req);
>> + dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
>
> Please check the return value if error happen, just print the err with dev_err()
> without stopping the release steps.
OK, will print errors
>> kfree(devfreq->time_in_state);
>> kfree(devfreq->trans_table);
>> mutex_destroy(&devfreq->lock);
>> kfree(devfreq);
>> }
>> @@ -747,18 +745,25 @@ struct devfreq *devfreq_add_device(struct device *dev,
>> devfreq->scaling_min_freq = find_available_min_freq(devfreq);
>> if (!devfreq->scaling_min_freq) {
>> err = -EINVAL;
>> goto err_dev;
>> }
>> - devfreq->min_freq = devfreq->scaling_min_freq;
>>
>> devfreq->scaling_max_freq = find_available_max_freq(devfreq);
>> if (!devfreq->scaling_max_freq) {
>> err = -EINVAL;
>> goto err_dev;
>> }
>> - devfreq->max_freq = devfreq->scaling_max_freq;
>> +
>> + err = dev_pm_qos_add_request(dev, &devfreq->user_min_freq_req,
>> + DEV_PM_QOS_MIN_FREQUENCY, 0);
>> + if (err < 0)
>> + goto err_dev;
>> + err = dev_pm_qos_add_request(dev, &devfreq->user_max_freq_req,
>> + DEV_PM_QOS_MAX_FREQUENCY, S32_MAX);
>> + if (err < 0)
>> + goto err_dev;
>>
>> devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
>> atomic_set(&devfreq->suspend_count, 0);
>>
>> devfreq->trans_table = kzalloc(
>> @@ -843,10 +848,14 @@ struct devfreq *devfreq_add_device(struct device *dev,
>> err_dev:
>> /*
>> * Cleanup path for errors that happen before registration.
>> * Otherwise we rely on devfreq_dev_release.
>> */
>> + if (dev_pm_qos_request_active(&devfreq->user_max_freq_req))
>> + dev_pm_qos_remove_request(&devfreq->user_max_freq_req);
>
> Please check the return value if error happen, just print the err with dev_err()
> without stopping the release steps.
OK, will print errors
>
> dev_err(... "failed to remove request of DEV_PM_QOS_MAX_FREQUENCY\n");
>
>> + if (dev_pm_qos_request_active(&devfreq->user_min_freq_req))
>> + dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
>
> dev_err(... "failed to remove request of DEV_PM_QOS_MIN_FREQUENCY\n");
>
>> kfree(devfreq->time_in_state);
>> kfree(devfreq->trans_table);
>> kfree(devfreq);
>> err_out:
>> return ERR_PTR(err);
>> @@ -1407,14 +1416,15 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
>>
>> ret = sscanf(buf, "%lu", &value);
>> if (ret != 1)
>> return -EINVAL;
>>
>> - mutex_lock(&df->lock);
>> - df->min_freq = value;
>> - update_devfreq(df);
>> - mutex_unlock(&df->lock);
>> + /* round down to kHz for PM QoS */
>
> I prefer more detailed description as following:
>
> /*
> * Round down to KHz to decide the proper minimum frequency
> * which is closed to user request.
> */
>
>
>> + ret = dev_pm_qos_update_request(&df->user_min_freq_req,
>> + value / HZ_PER_KHZ);
>> + if (ret < 0)
>> + return ret;
>>
>> return count;
>> }
>>
>> static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
>> @@ -1439,19 +1449,19 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>>
>> ret = sscanf(buf, "%lu", &value);
>> if (ret != 1)
>> return -EINVAL;
>>
>> - mutex_lock(&df->lock);
>> -
>> - /* Interpret zero as "don't care" */
>> - if (!value)
>> - value = ULONG_MAX;
>> + /* round up to kHz for PM QoS and interpret zero as "don't care" */
>
> I think that "don't care" comment style is not good.
>
> I referred to the Documentation/ABI/testing/sysfs-class-devfreq file.
> I prefer more detailed description as following:
> /*
> * Round up to KHz to decide the proper maximum frequency
> * which is closed to user request. If value is zero,
> * the user does not care.
> */
OK, will update this comment
>> + if (value)
>> + value = DIV_ROUND_UP(value, HZ_PER_KHZ);
>> + else
>> + value = S32_MAX;
>>
>> - df->max_freq = value;
>> - update_devfreq(df);
>> - mutex_unlock(&df->lock);
>> + ret = dev_pm_qos_update_request(&df->user_max_freq_req, value);
>> + if (ret < 0)
>> + return ret;
>>
>> return count;
>> }
>> static DEVICE_ATTR_RW(min_freq);
>>
>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>> index dac0dffeabb4..7849fe4c666d 100644
>> --- a/include/linux/devfreq.h
>> +++ b/include/linux/devfreq.h
>> @@ -11,10 +11,11 @@
>> #define __LINUX_DEVFREQ_H__
>>
>> #include <linux/device.h>
>> #include <linux/notifier.h>
>> #include <linux/pm_opp.h>
>> +#include <linux/pm_qos.h>
>>
>> #define DEVFREQ_NAME_LEN 16
>>
>> /* DEVFREQ governor name */
>> #define DEVFREQ_GOV_SIMPLE_ONDEMAND "simple_ondemand"
>> @@ -121,12 +122,12 @@ struct devfreq_dev_profile {
>> * devfreq.nb to the corresponding register notifier call chain.
>> * @work: delayed work for load monitoring.
>> * @previous_freq: previously configured frequency value.
>> * @data: Private data of the governor. The devfreq framework does not
>> * touch this.
>> - * @min_freq: Limit minimum frequency requested by user (0: none)
>> - * @max_freq: Limit maximum frequency requested by user (0: none)
>> + * @user_min_freq_req: PM QoS min frequency request from user (via sysfs)
>
> min -> minimum and then remove parenthesis as following:
> PM QoS minimum frequency request by user via sysfs
>
>> + * @user_max_freq_req: PM QoS max frequency request from user (via sysfs)
>
> ditto. max -> maximum
> PM QoS maximum frequency request by user via sysfs
OK
>> * @scaling_min_freq: Limit minimum frequency requested by OPP interface
>> * @scaling_max_freq: Limit maximum frequency requested by OPP interface
>> * @stop_polling: devfreq polling status of a device.
>> * @suspend_freq: frequency of a device set during suspend phase.
>> * @resume_freq: frequency of a device set in resume phase.
>> @@ -161,12 +162,12 @@ struct devfreq {
>> unsigned long previous_freq;
>> struct devfreq_dev_status last_status;
>>
>> void *data; /* private data for governors */
>>
>> - unsigned long min_freq;
>> - unsigned long max_freq;
>> + struct dev_pm_qos_request user_min_freq_req;
>> + struct dev_pm_qos_request user_max_freq_req;
>> unsigned long scaling_min_freq;
>> unsigned long scaling_max_freq;
>> bool stop_polling;
>>
>> unsigned long suspend_freq;
>>
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RFC PATCH 18/18] net: wireguard - switch to crypto API for packet encryption
From: Linus Torvalds @ 2019-09-25 22:15 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Jason A . Donenfeld, Catalin Marinas, Herbert Xu, Arnd Bergmann,
Eric Biggers, Greg KH, Samuel Neves, Linux Crypto Mailing List,
Andy Lutomirski, Marc Zyngier, Dan Carpenter, Will Deacon,
David Miller, Linux ARM
In-Reply-To: <20190925161255.1871-19-ard.biesheuvel@linaro.org>
On Wed, Sep 25, 2019 at 9:14 AM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
>
> Replace the chacha20poly1305() library calls with invocations of the
> RFC7539 AEAD, as implemented by the generic chacha20poly1305 template.
Honestly, the other patches look fine to me from what I've seen (with
the small note I had in a separate email for 11/18), but this one I
consider just nasty, and a prime example of why people hate those
crypto lookup routines.
Some of it is just the fundamental and pointless silly indirection,
that just makes things harder to read, less efficient, and less
straightforward.
That's exemplified by this part of the patch:
> struct noise_symmetric_key {
> - u8 key[NOISE_SYMMETRIC_KEY_LEN];
> + struct crypto_aead *tfm;
which is just one of those "we know what we want and we just want to
use it directly" things, and then the crypto indirection comes along
and makes that simple inline allocation of a small constant size
(afaik it is CHACHA20POLY1305_KEY_SIZE, which is 32) be another
allocation entirely.
And it's some random odd non-typed thing too, so then you have that
silly and stupid dynamic allocation using a name lookup:
crypto_alloc_aead("rfc7539(chacha20,poly1305)", 0, CRYPTO_ALG_ASYNC);
to create what used to be (and should be) a simple allocation that was
has a static type and was just part of the code.
It also ends up doing other bad things, ie that packet-time
+ if (unlikely(crypto_aead_reqsize(key->tfm) > 0)) {
+ req = aead_request_alloc(key->tfm, GFP_ATOMIC);
+ if (!req)
+ return false;
thing that hopefully _is_ unlikely, but that's just more potential
breakage from that whole async crypto interface.
This is what people do *not* want to do, and why people often don't
like the crypto interfaces.
It's exactly why we then have those "bare" routines as a library where
people just want to access the low-level hashing or whatever directly.
So please don't do things like this for an initial import.
Leave the thing alone, and just use the direct and synchronous static
crypto library interface tjhat you imported in patch 14/18 (but see
below about the incomplete import).
Now, later on, if you can *show* that some async implementation really
really helps, and you have numbers for it, and you can convince people
that the above kind of indirection is _worth_ it, then that's a second
phase. But don't make code uglier without those actual numbers.
Because it's not just uglier and has that silly extra indirection and
potential allocation problems, this part just looks very fragile
indeed:
> The nonce related changes are there to address the mismatch between the
> 96-bit nonce (aka IV) that the rfc7539() template expects, and the 64-bit
> nonce that WireGuard uses.
...
> struct packet_cb {
> - u64 nonce;
> - struct noise_keypair *keypair;
> atomic_t state;
> + __le32 ivpad; /* pad 64-bit nonce to 96 bits */
> + __le64 nonce;
> + struct noise_keypair *keypair;
> u32 mtu;
> u8 ds;
> };
The above is subtle and silently depends on the struct layout.
It really really shouldn't.
Can it be acceptable doing something like that? Yeah, but you really
should be making it very explicit, perhaps using
struct {
__le32 ivpad;
__le64 nonce;
} __packed;
or something like that.
Because right now you're depending on particular layout of those fields:
> + aead_request_set_crypt(req, sg, sg, skb->len,
> + (u8 *)&PACKET_CB(skb)->ivpad);
but honestly, that's not ok at all.
Somebody makes a slight change to that struct, and it might continue
to work fine on x86-32 (where 64-bit values are only 32-bit aligned)
but subtly break on other architectures.
Also, you changed how the nonce works from being in CPU byte order to
be explicitly LE. That may be ok, and looks like it might be a
cleanup, but honestly I think it should have been done as a separate
patch.
So could you please update that patch 14/18 to also have that
synchronous chacha20poly1305_decrypt_sg() interface, and then just
drop this 18/18 for now?
That would mean that
(a) you wouldn't need this patch, and you can then do that as a
separate second phase once you have numbers and it can stand on its
own.
(b) you'd actually have something that *builds* when you import the
main wireguard patch in 15/18
because right now it looks like you're not only forcing this async
interface with the unnecessary indirection, you're also basically
having a tree that doesn't even build or work for a couple of commits.
And I'm still not convinced (a) ever makes sense - the overhead of any
accelerator is just high enought that I doubt you'll have numbers -
performance _or_ power.
But even if you're right that it might be a power advantage on some
platform, that wouldn't make it an advantage on other platforms. Maybe
it could be done as a config option where you can opt in to the async
interface when that makes sense - but not force the indirection and
extra allocations when it doesn't. As a separate patch, something like
that doesn't sound horrendous (and I think that's also an argument for
doing that CPU->LE change as an independent change).
Yes, yes, there's also that 17/18 that switches over to a different
header file location and Kconfig names but that could easily be folded
into 15/18 and then it would all be bisectable.
Alternatively, maybe 15/18 could be done with wireguard disabled in
the Kconfig (just to make the patch identical), and then 17/18 enables
it when it compiles with a big note about how you wanted to keep 15/18
pristine to make the changes obvious.
Hmm?
I don't really have a dog in this fight, but on the whole I really
liked the series. But this 18/18 raised my heckles, and I think I
understand why it might raise the heckles of the wireguard people.
Please?
Linus
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RFC PATCH 18/18] net: wireguard - switch to crypto API for packet encryption
From: Linus Torvalds @ 2019-09-25 22:22 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Jason A . Donenfeld, Catalin Marinas, Herbert Xu, Arnd Bergmann,
Eric Biggers, Greg KH, Samuel Neves, Linux Crypto Mailing List,
Andy Lutomirski, Marc Zyngier, Dan Carpenter, Will Deacon,
David Miller, Linux ARM
In-Reply-To: <CAHk-=wjYsbxSiV_XKWV3BwGvau_hUvQiQHLOoc7vLUZt0Wqzfw@mail.gmail.com>
On Wed, Sep 25, 2019 at 3:15 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I don't really have a dog in this fight, but on the whole I really
> liked the series. But this 18/18 raised my heckles, and I think I
> understand why it might raise the heckles of the wireguard people.
To be honest, I guess I _do_ have a dog in the fight, namely the thing
that I'd love to see wireguard merged.
And this series otherwise looked non-offensive to me. Maybe not
everybody is hugely happy with all the details, but it looks like a
good sane "let's use as much of the existing models as possible" that
nobody should absolutely hate.
Linus
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 2/3] soc: amlogic: Add support for Secure power domains controller
From: Kevin Hilman @ 2019-09-25 22:41 UTC (permalink / raw)
To: Jianxin Pan, linux-amlogic
Cc: devicetree, Hanjie Lin, Victor Wan, Jianxin Pan, Neil Armstrong,
Martin Blumenstingl, linux-pm, linux-kernel, Zhiqiang Liang,
Rob Herring, Jian Hu, Xingyu Chen, linux-arm-kernel,
Jerome Brunet
In-Reply-To: <1568895064-4116-3-git-send-email-jianxin.pan@amlogic.com>
Hi Jianxin,
Jianxin Pan <jianxin.pan@amlogic.com> writes:
> Add support for the Amlogic Secure Power controller. In A1/C1 series, power
> control registers are in secure domain, and should be accessed by smc.
>
> Signed-off-by: Jianxin Pan <jianxin.pan@amlogic.com>
> Signed-off-by: Zhiqiang Liang <zhiqiang.liang@amlogic.com>
Thanks for the new power domain driver.
> ---
> drivers/soc/amlogic/Kconfig | 13 +++
> drivers/soc/amlogic/Makefile | 1 +
> drivers/soc/amlogic/meson-secure-pwrc.c | 182 ++++++++++++++++++++++++++++++++
> 3 files changed, 196 insertions(+)
> create mode 100644 drivers/soc/amlogic/meson-secure-pwrc.c
>
> diff --git a/drivers/soc/amlogic/Kconfig b/drivers/soc/amlogic/Kconfig
> index bc2c912..6cb06e7 100644
> --- a/drivers/soc/amlogic/Kconfig
> +++ b/drivers/soc/amlogic/Kconfig
> @@ -48,6 +48,19 @@ config MESON_EE_PM_DOMAINS
> Say yes to expose Amlogic Meson Everything-Else Power Domains as
> Generic Power Domains.
>
> +config MESON_SECURE_PM_DOMAINS
> + bool "Amlogic Meson Secure Power Domains driver"
> + depends on ARCH_MESON || COMPILE_TEST
> + depends on PM && OF
> + depends on HAVE_ARM_SMCCC
> + default ARCH_MESON
> + select PM_GENERIC_DOMAINS
> + select PM_GENERIC_DOMAINS_OF
> + help
> + Support for the power controller on Amlogic A1/C1 series.
> + Say yes to expose Amlogic Meson Secure Power Domains as Generic
> + Power Domains.
> +
> config MESON_MX_SOCINFO
> bool "Amlogic Meson MX SoC Information driver"
> depends on ARCH_MESON || COMPILE_TEST
> diff --git a/drivers/soc/amlogic/Makefile b/drivers/soc/amlogic/Makefile
> index de79d044..7b8c5d3 100644
> --- a/drivers/soc/amlogic/Makefile
> +++ b/drivers/soc/amlogic/Makefile
> @@ -5,3 +5,4 @@ obj-$(CONFIG_MESON_GX_SOCINFO) += meson-gx-socinfo.o
> obj-$(CONFIG_MESON_GX_PM_DOMAINS) += meson-gx-pwrc-vpu.o
> obj-$(CONFIG_MESON_MX_SOCINFO) += meson-mx-socinfo.o
> obj-$(CONFIG_MESON_EE_PM_DOMAINS) += meson-ee-pwrc.o
> +obj-$(CONFIG_MESON_SECURE_PM_DOMAINS) += meson-secure-pwrc.o
> diff --git a/drivers/soc/amlogic/meson-secure-pwrc.c b/drivers/soc/amlogic/meson-secure-pwrc.c
> new file mode 100644
> index 00000000..00c7232
> --- /dev/null
> +++ b/drivers/soc/amlogic/meson-secure-pwrc.c
> @@ -0,0 +1,182 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (c) 2019 Amlogic, Inc.
> + * Author: Jianxin Pan <jianxin.pan@amlogic.com>
> + */
> +#include <linux/io.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <dt-bindings/power/meson-a1-power.h>
> +#include <linux/arm-smccc.h>
> +
> +#define PWRC_ON 1
> +#define PWRC_OFF 0
> +#define SMC_PWRC_SET 0x82000093
> +#define SMC_PWRC_GET 0x82000095
> +
> +struct meson_secure_pwrc_domain {
> + struct generic_pm_domain base;
> + unsigned int index;
> +};
> +
> +struct meson_secure_pwrc {
> + struct meson_secure_pwrc_domain *domains;
> + struct genpd_onecell_data xlate;
> +};
> +
> +struct meson_secure_pwrc_domain_desc {
> + unsigned int index;
> + unsigned int flags;
> + char *name;
> + bool (*get_power)(struct meson_secure_pwrc_domain *pwrc_domain);
> +};
> +
> +struct meson_secure_pwrc_domain_data {
> + unsigned int count;
> + struct meson_secure_pwrc_domain_desc *domains;
> +};
> +
> +static bool pwrc_secure_get_power(struct meson_secure_pwrc_domain *pwrc_domain)
> +{
> + struct arm_smccc_res res;
> +
> + arm_smccc_smc(SMC_PWRC_GET, pwrc_domain->index, 0,
> + 0, 0, 0, 0, 0, &res);
> +
> + return res.a0 & 0x1;
Please use a #define with a readable name for this mask.
> +}
What does the return value for this function mean? Does true mean
"powered off" or "powered on"?
See the rename I just did on the ee-pwrc driver:
https://lore.kernel.org/linux-amlogic/20190925213528.21515-2-khilman@kernel.org/
> +static int meson_secure_pwrc_off(struct generic_pm_domain *domain)
> +{
> + struct arm_smccc_res res;
> + struct meson_secure_pwrc_domain *pwrc_domain =
> + container_of(domain, struct meson_secure_pwrc_domain, base);
> +
> + arm_smccc_smc(SMC_PWRC_SET, pwrc_domain->index, PWRC_OFF,
> + 0, 0, 0, 0, 0, &res);
> +
> + return 0;
> +}
> +
> +static int meson_secure_pwrc_on(struct generic_pm_domain *domain)
> +{
> + struct arm_smccc_res res;
> + struct meson_secure_pwrc_domain *pwrc_domain =
> + container_of(domain, struct meson_secure_pwrc_domain, base);
> +
> + arm_smccc_smc(SMC_PWRC_SET, pwrc_domain->index, PWRC_ON,
> + 0, 0, 0, 0, 0, &res);
> +
> + return 0;
> +}
> +
> +#define SEC_PD(__name, __flag) \
> +{ \
> + .name = #__name, \
> + .index = PWRC_##__name##_ID, \
> + .get_power = pwrc_secure_get_power, \
> + .flags = __flag, \
> +}
> +
> +static struct meson_secure_pwrc_domain_desc a1_pwrc_domains[] = {
> + SEC_PD(DSPA, 0),
> + SEC_PD(DSPB, 0),
> + SEC_PD(UART, GENPD_FLAG_ALWAYS_ON),
This flag should only be used for domains where there are no linux
drivers.
Rather than using this flag, you need to add a 'power-domain' property
to the uart driver in DT, and then update the meson_uart driver to use
the runtime PM API so that the domain is enabled whenever the UART is in
use.
> + SEC_PD(DMC, GENPD_FLAG_ALWAYS_ON),
Please explain the need for ALWAYS_ON.
> + SEC_PD(I2C, 0),
> + SEC_PD(PSRAM, 0),
> + SEC_PD(ACODEC, 0),
> + SEC_PD(AUDIO, 0),
> + SEC_PD(OTP, 0),
> + SEC_PD(DMA, 0),
> + SEC_PD(SD_EMMC, 0),
> + SEC_PD(RAMA, 0),
> + SEC_PD(RAMB, GENPD_FLAG_ALWAYS_ON),
Please explain the need for ALWAYS_ON.
> + SEC_PD(IR, 0),
> + SEC_PD(SPICC, 0),
> + SEC_PD(SPIFC, 0),
> + SEC_PD(USB, 0),
> + SEC_PD(NIC, GENPD_FLAG_ALWAYS_ON),
Please explain the need for ALWAYS_ON.
> + SEC_PD(PDMIN, 0),
> + SEC_PD(RSA, 0),
> +};
> +
> +static int meson_secure_pwrc_probe(struct platform_device *pdev)
> +{
> + const struct meson_secure_pwrc_domain_data *match;
> + struct meson_secure_pwrc *pwrc;
> + int i;
> +
> + match = of_device_get_match_data(&pdev->dev);
> + if (!match) {
> + dev_err(&pdev->dev, "failed to get match data\n");
> + return -ENODEV;
> + }
> +
> + pwrc = devm_kzalloc(&pdev->dev, sizeof(*pwrc), GFP_KERNEL);
> + if (!pwrc)
> + return -ENOMEM;
> +
> + pwrc->xlate.domains = devm_kcalloc(&pdev->dev, match->count,
> + sizeof(*pwrc->xlate.domains),
> + GFP_KERNEL);
> + if (!pwrc->xlate.domains)
> + return -ENOMEM;
> +
> + pwrc->domains = devm_kcalloc(&pdev->dev, match->count,
> + sizeof(*pwrc->domains), GFP_KERNEL);
> + if (!pwrc->domains)
> + return -ENOMEM;
> +
> + pwrc->xlate.num_domains = match->count;
> + platform_set_drvdata(pdev, pwrc);
> +
> + for (i = 0 ; i < match->count ; ++i) {
> + struct meson_secure_pwrc_domain *dom = &pwrc->domains[i];
> +
> + if (!match->domains[i].index)
> + continue;
> +
> + dom->index = match->domains[i].index;
> + dom->base.name = match->domains[i].name;
> + dom->base.flags = match->domains[i].flags;
> + dom->base.power_on = meson_secure_pwrc_on;
> + dom->base.power_off = meson_secure_pwrc_off;
> +
> + pm_genpd_init(&dom->base, NULL,
> + (match->domains[i].get_power ?
> + match->domains[i].get_power(dom) : true));
> +
> + pwrc->xlate.domains[i] = &dom->base;
> + }
> +
> + return of_genpd_add_provider_onecell(pdev->dev.of_node, &pwrc->xlate);
> +}
> +
> +static struct meson_secure_pwrc_domain_data meson_secure_a1_pwrc_data = {
> + .domains = a1_pwrc_domains,
> + .count = ARRAY_SIZE(a1_pwrc_domains),
> +};
> +
> +static const struct of_device_id meson_secure_pwrc_match_table[] = {
> + {
> + .compatible = "amlogic,meson-a1-pwrc",
> + .data = &meson_secure_a1_pwrc_data,
> + },
> + { }
as mentioned by Martin, please add the sentinel string here. Helps for
readability.
> +};
> +
> +static struct platform_driver meson_secure_pwrc_driver = {
> + .probe = meson_secure_pwrc_probe,
> + .driver = {
> + .name = "meson_secure_pwrc",
> + .of_match_table = meson_secure_pwrc_match_table,
> + },
> +};
> +
> +static int meson_secure_pwrc_init(void)
> +{
> + return platform_driver_register(&meson_secure_pwrc_driver);
> +}
> +arch_initcall_sync(meson_secure_pwrc_init);
Please use builtin_platform_driver() or explain in detail why the
initcall is needed.
Thanks,
Kevin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 0/5] provide the XTAL clock via OF on Meson8/8b/8m2
From: Kevin Hilman @ 2019-09-25 22:47 UTC (permalink / raw)
To: Martin Blumenstingl, Jerome Brunet
Cc: mark.rutland, devicetree, Neil Armstrong, linux-kernel, robh+dt,
linux-amlogic, linux-clk, linux-arm-kernel
In-Reply-To: <CAFBinCA0NaCJEDfNEg+LRfW3wxfNFGbXmGS+z7D5792TsupVAA@mail.gmail.com>
Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
> Hi Jerome,
>
> On Mon, Sep 23, 2019 at 11:29 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
>>
>> On Sat 21 Sep 2019 at 17:12, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:
>>
>> > So far the HHI clock controller has been providing the XTAL clock on
>> > Amlogic Meson8/Meson8b/Meson8m2 SoCs.
>> > This is not correct because the XTAL is actually a crystal on the
>> > boards and the SoC has a dedicated input for it.
>> >
>> > This updates the dt-bindings of the HHI clock controller and defines
>> > a fixed-clock in meson.dtsi (along with switching everything over to
>> > use this clock).
>> > The clock driver needs three updates to use this:
>> > - patch #2 uses clk_hw_set_parent in the CPU clock notifier. This drops
>> > the explicit reference to CLKID_XTAL while at the same time making
>> > the code much easier (thanks to Neil for providing this new method
>> > as part of the G12A CPU clock bringup!)
>> > - patch #3 ensures that the clock driver doesn't rely on it's internal
>> > XTAL clock while not losing support for older .dtbs that don't have
>> > the XTAL clock input yet
>> > - with patch #4 the clock controller's own XTAL clock is not registered
>> > anymore when a clock input is provided via OF
>> >
>> > This series is a functional no-op. It's main goal is to better represent
>> > how the actual hardware looks like.
>>
>> I'm a bit unsure about this series.
>>
>> On one hand, I totally agree with you ... having the xtal in DT is the
>> right way to do it ... when done from the start
> yep
>
>> On the other hand, things have been this way for years, they are working
>> and going for xtal in DT does not solve any pending issue. Doing this
>> means adding complexity in the driver to support both methods. It is
>> also quite a significant change in DT :/
> my two main motivations were:
> - keeping the 32-bit SoCs as similar as possible to the 64-bit ones in
> terms of "how are the [clock] drivers implemented"
> - with the DDR clock controller the .dts looked weird: &ddr_clkc took
> CLKID_XTAL from &clkc as input and &clkc took DDR_CLKID_DDR_PLL as
> input from &ddr_clkc
>
> RE complexity in the driver to support both:
> I still have a cleanup of the meson8b.c init code on my TODO-list
> because we're still supporting .dtbs without parent syscon
> my plan is to drop that code-path along with the newly added fallback
> for "skip CLKID_XTAL" (assuming this is accepted) together for v5.6 or
> v5.7
TBH, I'm big(ish) "functional no-op" changes like this are not things I
get super exicted about, especially for SoCs that have been working well
for awhile, and are do not have a large (upstream) userbase.
OTOH, since Martin is doing most of the heavy lifting for keeping this
platform working upstream, I'm happy to take the changes, as long as
Martin is willing to deal with any fallout.
Kevin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RFCv4 0/7] interconnect: Add imx support via devfreq
From: Leonard Crestez @ 2019-09-25 22:52 UTC (permalink / raw)
To: Georgi Djakov, Rob Herring, Artur Świgoń, Chanwoo Choi
Cc: Mark Rutland, devicetree@vger.kernel.org, Saravana Kannan,
linux-pm@vger.kernel.org, Stephen Boyd, Viresh Kumar,
Michael Turquette, Krzysztof Kozlowski, Kyungmin Park,
MyungJoo Ham, Alexandre Bailon, kernel@pengutronix.de,
Fabio Estevam, Shawn Guo, Aisheng Dong,
linux-arm-kernel@lists.infradead.org, dl-linux-imx
In-Reply-To: <02d3fe6a-53a6-3290-deab-d79e940978ff@linaro.org>
On 25.09.2019 05:38, Georgi Djakov wrote:
> Hi Leonard,
>
> On 9/16/19 05:34, Leonard Crestez wrote:
>> On 23.08.2019 17:37, Leonard Crestez wrote:
>>> This series add imx support for interconnect via devfreq: the ICC
>>> framework is used to aggregate requests from devices and then those are
>>> converted to DEV_PM_QOS_MIN_FREQUENCY requests for devfreq.
>>>
>>> Since there is no single devicetree node that can represent the "interconnect"
>>> new API is added to allow individual devfreq nodes to act as parsing proxies
>>> all mapping to a single soc-level icc provider. This is still RFC
>>> because of this
>>
>> Any comments? I made a lot of changes relative to previous versions,
>> most of them solely to avoid adding a virtual node in DT bindings.
>>
>> The only current interconnect provider implementation is for qcom and it
>> uses a firmware node as the provider node (with #interconnect-cells).
>> However there is no obvious equivalent of that for imx and many other SOCs.
>
> Not sure if it will help, but have you seen the qcs404 interconnect driver?
> There is also mt8183 interconnect provider driver on LKML.
Yes, but only yesterday. The qcs404 driver involves multiple DT devices
so it seems closer to imx.
As far as I understand from reading qcs404 source:
* There is no struct device representing the entire graph.
* There are multiple NOCs and each registers itself as a separate
interconnect provider.
* Each NOC registers multiple icc_nodes of various sorts:
* Device masters and slaves
* Some nodes representing NoC ports?
* Multiple internal nodes
* There is single per-SOC master list of QNOCs in the qcs404 driver.
* The QNOCs can reference each other between multiple providers.
* Each NOC registers an icc_provider and a subset of the graph.
* The multiple NoC inside a chip are distinguished by compat strings.
This seems strange, aren't they really different instantiations of the
same IP with small config changes?
This design is still quite odd, what would make sense to me is to
register the "interconnect graph" once and then provide multiple
"interconnect scalers" which handle the aggregated requests for certain
specific nodes.
>> On imx there are multiple pieces of scalable fabric which can be defined
>> in DT as devfreq devices and it sort of makes sense to add
>> #interconnect-cells to those. However when it comes to describing the
>> SOC interconnect graph it's much more convenient to have a single
>> per-SOC platform driver.
>
> Is all the NoC configuration done only by ATF? Are there any NoC related memory
> mapped registers?
Registers are memory-mapped and visible to the A-cores but should only
be accessed through secure transactions. This means that configuration
needs be done by ATF in EL3 (we don't support running linux in secure
world on imx8m). There is no "remote processor" managing this on imx8m.
On older imx6/7 chips we actually have two out-of-tree implementations
of bus freq switching code: An older one in Linux (used when running in
secure world) and a different one in optee for running Linux in
non-secure world.
NoC registers can be used to control some "transaction priority" bits
but I don't want to expose that part right now.
What determines bandwidth versus power consumption is the NoC clk rate
and clocks are managed by Linux directly.
DVFS on the RAM controller (DDRC) is also important. That component is
only a bus slave and frequency switching requires a complex sequence
inside ATF.
--
Regards,
Leonard
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 2/3] dt-bindings: reset: add bindings for the Meson-A1 SoC Reset Controller
From: Kevin Hilman @ 2019-09-25 22:55 UTC (permalink / raw)
To: Xingyu Chen, Philipp Zabel, Neil Armstrong
Cc: devicetree, Hanjie Lin, Jianxin Pan, linux-kernel, Rob Herring,
linux-arm-kernel, linux-amlogic, Xingyu Chen, Jerome Brunet
In-Reply-To: <1569227661-4261-3-git-send-email-xingyu.chen@amlogic.com>
Xingyu Chen <xingyu.chen@amlogic.com> writes:
> Add DT bindings for the Meson-A1 SoC Reset Controller include file,
> and also slightly update documentation.
>
> Signed-off-by: Xingyu Chen <xingyu.chen@amlogic.com>
> Signed-off-by: Jianxin Pan <jianxin.pan@amlogic.com>
The order here doesn't look right. As the sender, your sign-off should
be last. Is Jianxin the author or are you? If Jianxin, there should be
a "From:" line at the beginning of the changelog to indicate authorship
that's different from the sender.
> ---
> .../bindings/reset/amlogic,meson-reset.yaml | 1 +
> include/dt-bindings/reset/amlogic,meson-a1-reset.h | 59 ++++++++++++++++++++++
> 2 files changed, 60 insertions(+)
> create mode 100644 include/dt-bindings/reset/amlogic,meson-a1-reset.h
>
> diff --git a/Documentation/devicetree/bindings/reset/amlogic,meson-reset.yaml b/Documentation/devicetree/bindings/reset/amlogic,meson-reset.yaml
> index 00917d8..b3f57d8 100644
> --- a/Documentation/devicetree/bindings/reset/amlogic,meson-reset.yaml
> +++ b/Documentation/devicetree/bindings/reset/amlogic,meson-reset.yaml
> @@ -16,6 +16,7 @@ properties:
> - amlogic,meson8b-reset # Reset Controller on Meson8b and compatible SoCs
> - amlogic,meson-gxbb-reset # Reset Controller on GXBB and compatible SoCs
> - amlogic,meson-axg-reset # Reset Controller on AXG and compatible SoCs
> + - amlogic,meson-a1-reset # Reset Controller on A1 and compatible SoCs
>
> reg:
> maxItems: 1
> diff --git a/include/dt-bindings/reset/amlogic,meson-a1-reset.h b/include/dt-bindings/reset/amlogic,meson-a1-reset.h
> new file mode 100644
> index 00000000..8d76a47
> --- /dev/null
> +++ b/include/dt-bindings/reset/amlogic,meson-a1-reset.h
> @@ -0,0 +1,59 @@
> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> + *
> + * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
> + * Author: Xingyu Chen <xingyu.chen@amlogic.com>
> + *
> + */
> +
> +#ifndef _DT_BINDINGS_AMLOGIC_MESON_A1_RESET_H
> +#define _DT_BINDINGS_AMLOGIC_MESON_A1_RESET_H
> +
> +/* RESET0 */
> +#define RESET_AM2AXI_VAD 1
minor nit: can you use comments/whitespace here to indicate holes?
Please see the other amlogic files in this dir for examples.
Kevin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 3/3] reset: add support for the Meson-A1 SoC Reset Controller
From: Kevin Hilman @ 2019-09-25 22:57 UTC (permalink / raw)
To: Xingyu Chen, Philipp Zabel, Neil Armstrong
Cc: Hanjie Lin, Jianxin Pan, linux-kernel, Rob Herring,
linux-arm-kernel, linux-amlogic, Xingyu Chen, Jerome Brunet
In-Reply-To: <1569227661-4261-4-git-send-email-xingyu.chen@amlogic.com>
Hi Xingyu,
Xingyu Chen <xingyu.chen@amlogic.com> writes:
> The number of RESET registers and offset of RESET_LEVEL register for
> Meson-A1 are different from previous SoCs, In order to describe these
> differences, we introduce the struct meson_reset_param.
>
> Signed-off-by: Xingyu Chen <xingyu.chen@amlogic.com>
> Signed-off-by: Jianxin Pan <jianxin.pan@amlogic.com>
> Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
Again, order here isn't quite right. Add the reviewed-by tags first,
and the sender should be the last sign-off.
Other than that, driver looks good.
Reviewed-by: Kevin Hilman <khilman@baylibre.com>
Kevin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH v8 0/4] Panel rotation patches
From: Derek Basehore @ 2019-09-25 22:58 UTC (permalink / raw)
To: linux-kernel
Cc: Derek Basehore, Philipp Zabel, Maxime Ripard, Sam Ravnborg,
intel-gfx, Joonas Lahtinen, Maarten Lankhorst, Jani Nikula,
David Airlie, Thierry Reding, Matthias Brugger, dri-devel,
Daniel Vetter, Rodrigo Vivi, CK Hu, linux-mediatek, Sean Paul,
linux-arm-kernel
This adds the plumbing for reading panel rotation from the devicetree
and sets up adding a panel property for the panel orientation on
Mediatek SoCs when a rotation is present.
v8 changes:
-added reviewed-by tags
-fixed conflict with i915 patch that recently landed
-Added additional documentation
v7 changes:
-forgot to add static inline
v6 changes:
-added enum declaration to drm_panel.h header
v5 changes:
-rebased
v4 changes:
-fixed some changes made to the i915 driver
-clarified comments on of orientation helper
v3 changes:
-changed from attach/detach callbacks to directly setting fixed panel
values in drm_panel_attach
-removed update to Documentation
-added separate function for quirked panel orientation property init
v2 changes:
fixed build errors in i915
Derek Basehore (4):
drm/panel: Add helper for reading DT rotation
drm/panel: set display info in panel attach
drm/connector: Split out orientation quirk detection
drm/mtk: add panel orientation property
drivers/gpu/drm/drm_connector.c | 45 ++++++++++++++-----
drivers/gpu/drm/drm_panel.c | 70 ++++++++++++++++++++++++++++++
drivers/gpu/drm/i915/intel_dp.c | 4 +-
drivers/gpu/drm/i915/vlv_dsi.c | 5 +--
drivers/gpu/drm/mediatek/mtk_dsi.c | 8 ++++
include/drm/drm_connector.h | 2 +
include/drm/drm_panel.h | 21 +++++++++
7 files changed, 138 insertions(+), 17 deletions(-)
--
2.22.0.410.gd8fdbe21b5-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH v8 1/4] drm/panel: Add helper for reading DT rotation
From: Derek Basehore @ 2019-09-25 22:58 UTC (permalink / raw)
To: linux-kernel
Cc: Derek Basehore, Philipp Zabel, Maxime Ripard, Sam Ravnborg,
intel-gfx, Joonas Lahtinen, Maarten Lankhorst, Jani Nikula,
David Airlie, Thierry Reding, Matthias Brugger, dri-devel,
Daniel Vetter, Rodrigo Vivi, CK Hu, linux-mediatek, Sean Paul,
linux-arm-kernel
In-Reply-To: <20190925225833.7310-1-dbasehore@chromium.org>
This adds a helper function for reading the rotation (panel
orientation) from the device tree.
Signed-off-by: Derek Basehore <dbasehore@chromium.org>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
---
drivers/gpu/drm/drm_panel.c | 43 +++++++++++++++++++++++++++++++++++++
include/drm/drm_panel.h | 9 ++++++++
2 files changed, 52 insertions(+)
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index 6b0bf42039cf..0909b53b74e6 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -264,6 +264,49 @@ struct drm_panel *of_drm_find_panel(const struct device_node *np)
return ERR_PTR(-EPROBE_DEFER);
}
EXPORT_SYMBOL(of_drm_find_panel);
+
+/**
+ * of_drm_get_panel_orientation - look up the orientation of the panel through
+ * the "rotation" binding from a device tree node
+ * @np: device tree node of the panel
+ * @orientation: orientation enum to be filled in
+ *
+ * Looks up the rotation of a panel in the device tree. The orientation of the
+ * panel is expressed as a property name "rotation" in the device tree. The
+ * rotation in the device tree is counter clockwise.
+ *
+ * Return: 0 when a valid rotation value (0, 90, 180, or 270) is read or the
+ * rotation property doesn't exist. -EERROR otherwise.
+ */
+int of_drm_get_panel_orientation(const struct device_node *np,
+ enum drm_panel_orientation *orientation)
+{
+ int rotation, ret;
+
+ ret = of_property_read_u32(np, "rotation", &rotation);
+ if (ret == -EINVAL) {
+ /* Don't return an error if there's no rotation property. */
+ *orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
+ return 0;
+ }
+
+ if (ret < 0)
+ return ret;
+
+ if (rotation == 0)
+ *orientation = DRM_MODE_PANEL_ORIENTATION_NORMAL;
+ else if (rotation == 90)
+ *orientation = DRM_MODE_PANEL_ORIENTATION_RIGHT_UP;
+ else if (rotation == 180)
+ *orientation = DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP;
+ else if (rotation == 270)
+ *orientation = DRM_MODE_PANEL_ORIENTATION_LEFT_UP;
+ else
+ return -EINVAL;
+
+ return 0;
+}
+EXPORT_SYMBOL(of_drm_get_panel_orientation);
#endif
MODULE_AUTHOR("Thierry Reding <treding@nvidia.com>");
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
index 624bd15ecfab..d16158deacdc 100644
--- a/include/drm/drm_panel.h
+++ b/include/drm/drm_panel.h
@@ -34,6 +34,8 @@ struct drm_device;
struct drm_panel;
struct display_timing;
+enum drm_panel_orientation;
+
/**
* struct drm_panel_funcs - perform operations on a given panel
*
@@ -165,11 +167,18 @@ int drm_panel_get_modes(struct drm_panel *panel);
#if defined(CONFIG_OF) && defined(CONFIG_DRM_PANEL)
struct drm_panel *of_drm_find_panel(const struct device_node *np);
+int of_drm_get_panel_orientation(const struct device_node *np,
+ enum drm_panel_orientation *orientation);
#else
static inline struct drm_panel *of_drm_find_panel(const struct device_node *np)
{
return ERR_PTR(-ENODEV);
}
+static inline int of_drm_get_panel_orientation(const struct device_node *np,
+ enum drm_panel_orientation *orientation)
+{
+ return -ENODEV;
+}
#endif
#endif
--
2.23.0.351.gc4317032e6-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH v8 2/4] drm/panel: set display info in panel attach
From: Derek Basehore @ 2019-09-25 22:58 UTC (permalink / raw)
To: linux-kernel
Cc: Derek Basehore, Philipp Zabel, Maxime Ripard, Sam Ravnborg,
intel-gfx, Joonas Lahtinen, Maarten Lankhorst, Jani Nikula,
David Airlie, Thierry Reding, Matthias Brugger, dri-devel,
Daniel Vetter, Rodrigo Vivi, CK Hu, linux-mediatek, Sean Paul,
linux-arm-kernel
In-Reply-To: <20190925225833.7310-1-dbasehore@chromium.org>
Devicetree systems can set panel orientation via a panel binding, but
there's no way, as is, to propagate this setting to the connector,
where the property need to be added.
To address this, this patch sets orientation, as well as other fixed
values for the panel, in the drm_panel_attach function. These values
are stored from probe in the drm_panel struct.
Signed-off-by: Derek Basehore <dbasehore@chromium.org>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
---
drivers/gpu/drm/drm_panel.c | 28 +++++++++++++++++++++
include/drm/drm_panel.h | 50 +++++++++++++++++++++++++++++++++++++
2 files changed, 78 insertions(+)
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index 0909b53b74e6..1cd2b56c9fe6 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -104,11 +104,23 @@ EXPORT_SYMBOL(drm_panel_remove);
*/
int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector)
{
+ struct drm_display_info *info;
+
if (panel->connector)
return -EBUSY;
panel->connector = connector;
panel->drm = connector->dev;
+ info = &connector->display_info;
+ info->width_mm = panel->width_mm;
+ info->height_mm = panel->height_mm;
+ info->bpc = panel->bpc;
+ info->panel_orientation = panel->orientation;
+ info->bus_flags = panel->bus_flags;
+ if (panel->bus_formats)
+ drm_display_info_set_bus_formats(&connector->display_info,
+ panel->bus_formats,
+ panel->num_bus_formats);
return 0;
}
@@ -126,6 +138,22 @@ EXPORT_SYMBOL(drm_panel_attach);
*/
void drm_panel_detach(struct drm_panel *panel)
{
+ struct drm_display_info *info;
+
+ if (!panel->connector)
+ goto out;
+
+ info = &panel->connector->display_info;
+ info->width_mm = 0;
+ info->height_mm = 0;
+ info->bpc = 0;
+ info->panel_orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
+ info->bus_flags = 0;
+ kfree(info->bus_formats);
+ info->bus_formats = NULL;
+ info->num_bus_formats = 0;
+
+out:
panel->connector = NULL;
panel->drm = NULL;
}
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
index d16158deacdc..f3587a54b8ac 100644
--- a/include/drm/drm_panel.h
+++ b/include/drm/drm_panel.h
@@ -141,6 +141,56 @@ struct drm_panel {
*/
const struct drm_panel_funcs *funcs;
+ /**
+ * @width_mm:
+ *
+ * Physical width in mm.
+ */
+ unsigned int width_mm;
+
+ /**
+ * @height_mm:
+ *
+ * Physical height in mm.
+ */
+ unsigned int height_mm;
+
+ /**
+ * @bpc:
+ *
+ * Maximum bits per color channel. Used by HDMI and DP outputs.
+ */
+ unsigned int bpc;
+
+ /**
+ * @orientation
+ *
+ * Installation orientation of the panel with respect to the chassis.
+ */
+ int orientation;
+
+ /**
+ * @bus_formats
+ *
+ * Pixel data format on the wire.
+ */
+ const u32 *bus_formats;
+
+ /**
+ * @num_bus_formats:
+ *
+ * Number of elements pointed to by @bus_formats
+ */
+ unsigned int num_bus_formats;
+
+ /**
+ * @bus_flags:
+ *
+ * Additional information (like pixel signal polarity) for the pixel
+ * data on the bus.
+ */
+ u32 bus_flags;
+
/**
* @list:
*
--
2.23.0.351.gc4317032e6-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH v8 3/4] drm/connector: Split out orientation quirk detection
From: Derek Basehore @ 2019-09-25 22:58 UTC (permalink / raw)
To: linux-kernel
Cc: Derek Basehore, Philipp Zabel, Maxime Ripard, Sam Ravnborg,
intel-gfx, Joonas Lahtinen, Maarten Lankhorst, Jani Nikula,
David Airlie, Thierry Reding, Matthias Brugger, dri-devel,
Daniel Vetter, Rodrigo Vivi, CK Hu, linux-mediatek, Sean Paul,
linux-arm-kernel
In-Reply-To: <20190925225833.7310-1-dbasehore@chromium.org>
Not every platform needs quirk detection for panel orientation, so
split the drm_connector_init_panel_orientation_property into two
functions. One for platforms without the need for quirks, and the
other for platforms that need quirks.
Signed-off-by: Derek Basehore <dbasehore@chromium.org>
Acked-by: Sam Ravnborg <sam@ravnborg.org>
---
drivers/gpu/drm/drm_connector.c | 45 ++++++++++++++++++-------
drivers/gpu/drm/i915/display/icl_dsi.c | 2 +-
drivers/gpu/drm/i915/display/intel_dp.c | 4 +--
drivers/gpu/drm/i915/display/vlv_dsi.c | 2 +-
include/drm/drm_connector.h | 2 ++
5 files changed, 39 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 4c766624b20d..faef25683faf 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1989,31 +1989,23 @@ EXPORT_SYMBOL(drm_connector_set_vrr_capable_property);
* drm_connector_init_panel_orientation_property -
* initialize the connecters panel_orientation property
* @connector: connector for which to init the panel-orientation property.
- * @width: width in pixels of the panel, used for panel quirk detection
- * @height: height in pixels of the panel, used for panel quirk detection
*
* This function should only be called for built-in panels, after setting
* connector->display_info.panel_orientation first (if known).
*
- * This function will check for platform specific (e.g. DMI based) quirks
- * overriding display_info.panel_orientation first, then if panel_orientation
- * is not DRM_MODE_PANEL_ORIENTATION_UNKNOWN it will attach the
- * "panel orientation" property to the connector.
+ * This function will check if the panel_orientation is not
+ * DRM_MODE_PANEL_ORIENTATION_UNKNOWN. If not, it will attach the "panel
+ * orientation" property to the connector.
*
* Returns:
* Zero on success, negative errno on failure.
*/
int drm_connector_init_panel_orientation_property(
- struct drm_connector *connector, int width, int height)
+ struct drm_connector *connector)
{
struct drm_device *dev = connector->dev;
struct drm_display_info *info = &connector->display_info;
struct drm_property *prop;
- int orientation_quirk;
-
- orientation_quirk = drm_get_panel_orientation_quirk(width, height);
- if (orientation_quirk != DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
- info->panel_orientation = orientation_quirk;
if (info->panel_orientation == DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
return 0;
@@ -2036,6 +2028,35 @@ int drm_connector_init_panel_orientation_property(
}
EXPORT_SYMBOL(drm_connector_init_panel_orientation_property);
+/**
+ * drm_connector_init_panel_orientation_property_quirk -
+ * initialize the connecters panel_orientation property with a quirk
+ * override
+ * @connector: connector for which to init the panel-orientation property.
+ * @width: width in pixels of the panel, used for panel quirk detection
+ * @height: height in pixels of the panel, used for panel quirk detection
+ *
+ * This function will check for platform specific (e.g. DMI based) quirks
+ * overriding display_info.panel_orientation first, then if panel_orientation
+ * is not DRM_MODE_PANEL_ORIENTATION_UNKNOWN it will attach the
+ * "panel orientation" property to the connector.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_connector_init_panel_orientation_property_quirk(
+ struct drm_connector *connector, int width, int height)
+{
+ int orientation_quirk;
+
+ orientation_quirk = drm_get_panel_orientation_quirk(width, height);
+ if (orientation_quirk != DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
+ connector->display_info.panel_orientation = orientation_quirk;
+
+ return drm_connector_init_panel_orientation_property(connector);
+}
+EXPORT_SYMBOL(drm_connector_init_panel_orientation_property_quirk);
+
int drm_connector_set_obj_prop(struct drm_mode_object *obj,
struct drm_property *property,
uint64_t value)
diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c b/drivers/gpu/drm/i915/display/icl_dsi.c
index 6e398c33a524..483287984090 100644
--- a/drivers/gpu/drm/i915/display/icl_dsi.c
+++ b/drivers/gpu/drm/i915/display/icl_dsi.c
@@ -1538,7 +1538,7 @@ static void icl_dsi_add_properties(struct intel_connector *connector)
connector->base.display_info.panel_orientation =
intel_dsi_get_panel_orientation(connector);
- drm_connector_init_panel_orientation_property(&connector->base,
+ drm_connector_init_panel_orientation_property_quirk(&connector->base,
connector->panel.fixed_mode->hdisplay,
connector->panel.fixed_mode->vdisplay);
}
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 921ad0a2f7ba..419413fa8165 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -7076,8 +7076,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
intel_panel_setup_backlight(connector, pipe);
if (fixed_mode)
- drm_connector_init_panel_orientation_property(
- connector, fixed_mode->hdisplay, fixed_mode->vdisplay);
+ drm_connector_init_panel_orientation_property_quirk(connector,
+ fixed_mode->hdisplay, fixed_mode->vdisplay);
return true;
diff --git a/drivers/gpu/drm/i915/display/vlv_dsi.c b/drivers/gpu/drm/i915/display/vlv_dsi.c
index a71b22bdd95b..46cfb0821c17 100644
--- a/drivers/gpu/drm/i915/display/vlv_dsi.c
+++ b/drivers/gpu/drm/i915/display/vlv_dsi.c
@@ -1634,7 +1634,7 @@ static void vlv_dsi_add_properties(struct intel_connector *connector)
connector->base.display_info.panel_orientation =
vlv_dsi_get_panel_orientation(connector);
- drm_connector_init_panel_orientation_property(
+ drm_connector_init_panel_orientation_property_quirk(
&connector->base,
connector->panel.fixed_mode->hdisplay,
connector->panel.fixed_mode->vdisplay);
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 681cb590f952..e3416ac11478 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1540,6 +1540,8 @@ void drm_connector_set_link_status_property(struct drm_connector *connector,
void drm_connector_set_vrr_capable_property(
struct drm_connector *connector, bool capable);
int drm_connector_init_panel_orientation_property(
+ struct drm_connector *connector);
+int drm_connector_init_panel_orientation_property_quirk(
struct drm_connector *connector, int width, int height);
int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
int min, int max);
--
2.23.0.351.gc4317032e6-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH v8 4/4] drm/mtk: add panel orientation property
From: Derek Basehore @ 2019-09-25 22:58 UTC (permalink / raw)
To: linux-kernel
Cc: Derek Basehore, Philipp Zabel, Maxime Ripard, Sam Ravnborg,
intel-gfx, Joonas Lahtinen, Maarten Lankhorst, Jani Nikula,
David Airlie, Thierry Reding, Matthias Brugger, dri-devel,
Daniel Vetter, Rodrigo Vivi, CK Hu, linux-mediatek, Sean Paul,
linux-arm-kernel
In-Reply-To: <20190925225833.7310-1-dbasehore@chromium.org>
This inits the panel orientation property for the mediatek dsi driver
if the panel orientation (connector.display_info.panel_orientation) is
not DRM_MODE_PANEL_ORIENTATION_UNKNOWN.
Signed-off-by: Derek Basehore <dbasehore@chromium.org>
Acked-by: Sam Ravnborg <sam@ravnborg.org>
Reviewed-by: CK Hu <ck.hu@mediatek.com>
---
drivers/gpu/drm/mediatek/mtk_dsi.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index 224afb666881..2936932344eb 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -792,10 +792,18 @@ static int mtk_dsi_create_connector(struct drm_device *drm, struct mtk_dsi *dsi)
DRM_ERROR("Failed to attach panel to drm\n");
goto err_connector_cleanup;
}
+
+ ret = drm_connector_init_panel_orientation_property(&dsi->conn);
+ if (ret) {
+ DRM_ERROR("Failed to init panel orientation\n");
+ goto err_panel_detach;
+ }
}
return 0;
+err_panel_detach:
+ drm_panel_detach(dsi->panel);
err_connector_cleanup:
drm_connector_cleanup(&dsi->conn);
return ret;
--
2.23.0.351.gc4317032e6-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH] arm64: Allow disabling of the compat vDSO
From: Vincenzo Frascino @ 2019-09-25 23:35 UTC (permalink / raw)
To: Nick Desaulniers, Catalin Marinas
Cc: Thomas Gleixner, Will Deacon, LKML, Linux ARM, Ard Biesheuvel
In-Reply-To: <CAKwvOd=GcF0Tv2-h0LNMvCzx+tm5skKW1J7P=NTf8xYbmPiOPw@mail.gmail.com>
On 9/25/19 6:31 PM, Nick Desaulniers wrote:
> On Wed, Sep 25, 2019 at 10:08 AM Catalin Marinas
> <catalin.marinas@arm.com> wrote:
>>
>> This is just a temporary hiding of the issue, not a complete fix.
>
> Yep.
>
>> Vincenzo will do the fix later on.
>
> Appreciated, I'm happy to help discuss, review, and test.
>
>>>> - check whether COMPATCC is clang or not rather than CC_IS_CLANG, which
>>>> only checks the native compiler
>>>
>>> When cross compiling, IIUC CC_IS_CLANG is referring to CC which is the
>>> cross compiler, which is different than HOSTCC which is the host
>>> compiler. HOSTCC is used mostly for things in scripts/ while CC is
>>> used to compile a majority of the kernel in a cross compile.
>>
>> We need the third compiler here for the compat vDSO (at least with gcc),
>> COMPATCC. I'm tempted to just drop the CONFIG_CROSS_COMPILE_COMPAT_VDSO
>> altogether and only rely on a COMPATCC. This way we can add
>> COMPATCC_IS_CLANG etc. in the Kconfig checks directly.
>
> Oh, man, yeah 3 compilers in that case:
> 1. HOSTCC
> 2. CC
> 3. COMPATCC
>
The easier way I found to encapsulate the three compilers is using
CONFIG_CROSS_COMPILE_COMPAT_VDSO, hence I would prefer to not drop it.
In the case of gcc:
-------------------
CROSS_COMPILE_COMPAT ?= $(CONFIG_CROSS_COMPILE_COMPAT_VDSO:"%"=%)
$(CROSS_COMPILE_COMPAT)gcc ...
In the case of clang:
---------------------
CLANG_TRIPLE ?= $(CONFIG_CROSS_COMPILE_COMPAT_VDSO:"%-"=%)
clang --target=$(notdir $CLANG_TRIPLE)
This will prevent the vdso32 library generation to depend from a fixed
configuration that might change in future.
Based on this work we will be able to add COMPAT_CC_IS_CLANG, COMPAT_CC_IS_GCC
and COMPAT_CC_GCC_VERSION, COMPAT_CC_CLANG_VERSION which will help us in a more
fine grain control of the compiler versions.
The clang support will be added shortly after the header problems have been
addressed, because without that and the possibility to have 32bit headers in
arm64 would result in a broken target.
>>
>> If clang can build both 32 and 64-bit with the same binary (just
>> different options), we could maybe have COMPATCC default to CC and add a
>> check on whether COMPATCC generates 32-bit binaries.
>
> Cross compilation work differently between GCC and Clang from a
> developers perspective. In GCC, at ./configure time you select which
> architecture you'd like to cross compile for, and you get one binary
> that targets one architecture. You get a nice compiler that can do
> mostly static dispatch at the cost of needing multiple binaries in
> admittedly rare scenarios like the one we have here. Clang defaults
> to all backends enabled when invoking cmake (though there are options
> to enable certain backends; Sony for instance enables only x86_64 for
> their PS4 SDK (and thus I break their build frequently with my arm64
> unit tests)).
>
> Clang can do all of the above with one binary. The codebase makes
> heavy use of OOP+virtual dispatch to run ISA specific and general code
> transformations (virtual dispatch is slower than static dispatch, but
> relative to what the compiler is actually doing, I suspect the effects
> are minimal. Folks are also heavily invested in researching
> "devirtualization"). With one clang binary, I can do:
>
> # implicitly uses the host's ISA, for me that's x86_64-linux-gnu
> $ clang foo.c
> $ clang -target aarch64-linux-gnu foo.c
> $ clang -target arm-linux-gnueabi foo.c
>
> Admittedly, it's not as simple for the kernel's case; the top level
> Makefile sets some flags to support invoking Clang from a non-standard
> location, and telling it where to find binutils because we can't
> assemble the kernel with LLVM's substitute for GAS).
>
Thank you for the explanation, if I understand it correctly the strategy
proposed above should cover all the cased proposed. Please, let me know if i
need to tweak something.
The plan is to use binutils to build the vdso libraries with both the compilers.
--
Regards,
Vincenzo
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [arm-platforms:irq/gic-v4.1-devel 12/37] irq-gic-v3-its.c:undefined reference to `__aeabi_uldivmod'
From: kbuild test robot @ 2019-09-25 23:46 UTC (permalink / raw)
To: Marc Zyngier; +Cc: kbuild-all, linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 1110 bytes --]
tree: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git irq/gic-v4.1-devel
head: d0930710fc52cf43ff23432ac6c2ac98b4a8e3b2
commit: 332496d87708d4c745a13687538661952928227f [12/37] irqchip/gic-v4.1: VPE table (aka GICR_VPROPBASER) allocation
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 7.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout 332496d87708d4c745a13687538661952928227f
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=arm
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/irqchip/irq-gic-v3-its.o: In function `its_cpu_init':
>> irq-gic-v3-its.c:(.text+0x6534): undefined reference to `__aeabi_uldivmod'
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 71376 bytes --]
[-- Attachment #3: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] counter: stm32-timer-cnt: fix a kernel-doc warning
From: William Breathitt Gray @ 2019-09-25 23:49 UTC (permalink / raw)
To: Fabrice Gasnier, jic23
Cc: alexandre.torgue, linux-iio, linux-kernel, jic23, linux-stm32,
linux-arm-kernel
In-Reply-To: <1568809323-26079-1-git-send-email-fabrice.gasnier@st.com>
On Wed, Sep 18, 2019 at 02:22:03PM +0200, Fabrice Gasnier wrote:
> Fix the following warning when documentation is built:
> drivers/counter/stm32-timer-cnt.c:37: warning: cannot understand function
> prototype: 'enum stm32_count_function'
>
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> ---
> drivers/counter/stm32-timer-cnt.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c
> index 644ba18..e425dd1 100644
> --- a/drivers/counter/stm32-timer-cnt.c
> +++ b/drivers/counter/stm32-timer-cnt.c
> @@ -28,7 +28,7 @@ struct stm32_timer_cnt {
> };
>
> /**
> - * stm32_count_function - enumerates stm32 timer counter encoder modes
> + * enum stm32_count_function - enumerates stm32 timer counter encoder modes
> * @STM32_COUNT_SLAVE_MODE_DISABLED: counts on internal clock when CEN=1
> * @STM32_COUNT_ENCODER_MODE_1: counts TI1FP1 edges, depending on TI2FP2 level
> * @STM32_COUNT_ENCODER_MODE_2: counts TI2FP2 edges, depending on TI1FP1 level
> --
> 2.7.4
Fixes: 597f55e3f36c ("counter: stm32-lptimer: add counter device")
Jonathan, please pick this fix up through IIO.
Thanks,
William Breathitt Gray
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] counter: stm32-lptimer-cnt: fix a kernel-doc warning
From: William Breathitt Gray @ 2019-09-25 23:51 UTC (permalink / raw)
To: Fabrice Gasnier, jic23
Cc: alexandre.torgue, linux-iio, linux-kernel, jic23, linux-stm32,
linux-arm-kernel
In-Reply-To: <1568809361-26157-1-git-send-email-fabrice.gasnier@st.com>
On Wed, Sep 18, 2019 at 02:22:41PM +0200, Fabrice Gasnier wrote:
> Fix the following warnings when documentation is built:
> drivers/counter/stm32-lptimer-cnt.c:354: warning: cannot understand
> function prototype: 'enum stm32_lptim_cnt_function'
>
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> ---
> drivers/counter/stm32-lptimer-cnt.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/counter/stm32-lptimer-cnt.c b/drivers/counter/stm32-lptimer-cnt.c
> index bbc930a..28b6364 100644
> --- a/drivers/counter/stm32-lptimer-cnt.c
> +++ b/drivers/counter/stm32-lptimer-cnt.c
> @@ -347,7 +347,7 @@ static const struct iio_chan_spec stm32_lptim_cnt_channels = {
> };
>
> /**
> - * stm32_lptim_cnt_function - enumerates stm32 LPTimer counter & encoder modes
> + * enum stm32_lptim_cnt_function - enumerates LPTimer counter & encoder modes
> * @STM32_LPTIM_COUNTER_INCREASE: up count on IN1 rising, falling or both edges
> * @STM32_LPTIM_ENCODER_BOTH_EDGE: count on both edges (IN1 & IN2 quadrature)
> */
> --
> 2.7.4
Fixes: 597f55e3f36c ("counter: stm32-lptimer: add counter device")
Jonathan, please pick this fix up through IIO.
Thanks,
William Breathitt Gray
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] counter: stm32-timer-cnt: fix a kernel-doc warning
From: William Breathitt Gray @ 2019-09-25 23:53 UTC (permalink / raw)
To: Fabrice Gasnier, jic23
Cc: alexandre.torgue, linux-iio, linux-kernel, jic23, linux-stm32,
linux-arm-kernel
In-Reply-To: <20190925234927.GB14133@icarus>
On Wed, Sep 25, 2019 at 07:49:27PM -0400, William Breathitt Gray wrote:
> On Wed, Sep 18, 2019 at 02:22:03PM +0200, Fabrice Gasnier wrote:
> > Fix the following warning when documentation is built:
> > drivers/counter/stm32-timer-cnt.c:37: warning: cannot understand function
> > prototype: 'enum stm32_count_function'
> >
> > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> > ---
> > drivers/counter/stm32-timer-cnt.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c
> > index 644ba18..e425dd1 100644
> > --- a/drivers/counter/stm32-timer-cnt.c
> > +++ b/drivers/counter/stm32-timer-cnt.c
> > @@ -28,7 +28,7 @@ struct stm32_timer_cnt {
> > };
> >
> > /**
> > - * stm32_count_function - enumerates stm32 timer counter encoder modes
> > + * enum stm32_count_function - enumerates stm32 timer counter encoder modes
> > * @STM32_COUNT_SLAVE_MODE_DISABLED: counts on internal clock when CEN=1
> > * @STM32_COUNT_ENCODER_MODE_1: counts TI1FP1 edges, depending on TI2FP2 level
> > * @STM32_COUNT_ENCODER_MODE_2: counts TI2FP2 edges, depending on TI1FP1 level
> > --
> > 2.7.4
>
> Fixes: 597f55e3f36c ("counter: stm32-lptimer: add counter device")
>
> Jonathan, please pick this fix up through IIO.
>
> Thanks,
>
> William Breathitt Gray
Sorry, that's the wrong Fixes line. Here's the right one:
Fixes: ad29937e206f ("counter: Add STM32 Timer quadrature encoder")
William Breathitt Gray
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] arm64: Allow disabling of the compat vDSO
From: Vincenzo Frascino @ 2019-09-26 0:06 UTC (permalink / raw)
To: Catalin Marinas, Nick Desaulniers
Cc: Thomas Gleixner, Will Deacon, LKML, Linux ARM, Ard Biesheuvel
In-Reply-To: <20190925170838.GK7042@arrakis.emea.arm.com>
On 9/25/19 6:08 PM, Catalin Marinas wrote:
> On Wed, Sep 25, 2019 at 09:53:16AM -0700, Nick Desaulniers wrote:
>> On Wed, Sep 25, 2019 at 6:09 AM Catalin Marinas <catalin.marinas@arm.com> wrote:
>>>
>>> The compat vDSO building requires a cross-compiler than produces AArch32
>>> binaries, defined via CONFIG_CROSS_COMPILE_COMPAT_VDSO or the
>>> CROSS_COMPILE_COMPAT environment variable. If none of these is defined,
>>> building the kernel always prints a warning as there is no way to
>>> deselect the compat vDSO.
>>>
>>> Add an arm64 Kconfig entry to allow the deselection of the compat vDSO.
>>> In addition, make it an EXPERT option, default n, until other issues
>>> with the compat vDSO are solved (64-bit only kernel headers included in
>>> user-space vDSO code, CC_IS_CLANG irrelevant to CROSS_COMPILE_COMPAT).
>>
>> CC_IS_CLANG might be because then CC can be reused with different
>> flags, rather than providing a different cross compiler binary via
>> config option.
>>
>>>
>>> Fixes: bfe801ebe84f ("arm64: vdso: Enable vDSO compat support")
>>> Cc: Will Deacon <will@kernel.org>
>>> Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
>>
>> Thanks for the patch.
>> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
>> Link: https://github.com/ClangBuiltLinux/linux/issues/595
>
> This is just a temporary hiding of the issue, not a complete fix.
> Vincenzo will do the fix later on.
>
>> Overall, this work is important to Android; the ARMv8-A series of
>> mobile SoCs we see today have to support 32b and 64b (A32+A64?) for at
>> least a few more years; we would like gettimeofday() and friends to be
>> fast for 32b and 64b applications.
>
> I agree, it just needs some tweaking and hopefully we get most of it
> fixed in 5.4.
>
>>> Suggestions for future improvements of the compat vDSO handling:
>>>
>>> - replace the CROSS_COMPILE_COMPAT prefix with a full COMPATCC; maybe
>>> check that it indeed produces 32-bit code
>>>
CROSS_COMPILE_COMPAT is called like this for symmetry with CROSS_COMPILE.
>>> - check whether COMPATCC is clang or not rather than CC_IS_CLANG, which
>>> only checks the native compiler
>>
>> When cross compiling, IIUC CC_IS_CLANG is referring to CC which is the
>> cross compiler, which is different than HOSTCC which is the host
>> compiler. HOSTCC is used mostly for things in scripts/ while CC is
>> used to compile a majority of the kernel in a cross compile.
>
> We need the third compiler here for the compat vDSO (at least with gcc),
> COMPATCC. I'm tempted to just drop the CONFIG_CROSS_COMPILE_COMPAT_VDSO
> altogether and only rely on a COMPATCC. This way we can add
> COMPATCC_IS_CLANG etc. in the Kconfig checks directly.
>
> If clang can build both 32 and 64-bit with the same binary (just
> different options), we could maybe have COMPATCC default to CC and add a
> check on whether COMPATCC generates 32-bit binaries.
>
clang requires the --target option for specifying the 32bit triple.
Basically $(TRIPLE)-gcc is equivalent to gcc --target $(TRIPLE).
We need a configuration option to encode this.
>>> - clean up the headers includes; vDSO should not include kernel-only
>>> headers that may even contain code patched at run-time
>>
>> This is a big one; Clang validates the inline asm constraints for
>> extended inline assembly, GCC does not for dead code. So Clang chokes
>> on the inclusion of arm64 headers using extended inline assembly when
>> being compiled for arm-linux-gnueabi.
>
> Whether clang or gcc, I'd like this fixed anyway. At some point we may
> inadvertently rely on some code which is patched at boot time for the
> kernel code but not for the vDSO.
>
Do we have any code of this kind in header files?
The vDSO library uses only a subset of the headers (mainly Macros) hence all the
unused symbols should be compiled out. Is your concern only theoretical or do
you have an example on where this could be happening?
> Thanks.
>
--
Regards,
Vincenzo
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* RE: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
From: Anson Huang @ 2019-09-26 0:34 UTC (permalink / raw)
To: Leonard Crestez, shawnguo@kernel.org, Aisheng Dong
Cc: s.hauer@pengutronix.de, linux-kernel@vger.kernel.org,
dl-linux-imx, kernel@pengutronix.de, festevam@gmail.com,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <VI1PR04MB70232DEA67972332611480CAEE870@VI1PR04MB7023.eurprd04.prod.outlook.com>
Hi, Leonard
> On 25.09.2019 13:09, Anson Huang wrote:
> > The SCU firmware does NOT always have return value stored in message
> > header's function element even the API has response data, those
> > special APIs are defined as void function in SCU firmware, so they
> > should be treated as return success always.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> > - This patch is based on the patch of
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> >
> hwork.kernel.org%2Fpatch%2F11129553%2F&data=02%7C01%7Cleonar
> d.cres
> >
> tez%40nxp.com%7Cc0ced6cd07f04023977008d741a07367%7C686ea1d3bc2b
> 4c6fa92
> >
> cd99c5c301635%7C0%7C0%7C637050029712216472&sdata=Ccq%2Fb2R
> JdMqmnL7
> > VXrl8YhOlUwC7bWiUG%2BNmiw4OsSM%3D&reserved=0
> > ---
> > drivers/firmware/imx/imx-scu.c | 34
> ++++++++++++++++++++++++++++++++--
> > 1 file changed, 32 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/firmware/imx/imx-scu.c
> > b/drivers/firmware/imx/imx-scu.c index 869be7a..ced5b12 100644
> > --- a/drivers/firmware/imx/imx-scu.c
> > +++ b/drivers/firmware/imx/imx-scu.c
> > @@ -78,6 +78,11 @@ static int imx_sc_linux_errmap[IMX_SC_ERR_LAST] =
> {
> > -EIO, /* IMX_SC_ERR_FAIL */
> > };
> >
> > +static const struct imx_sc_rpc_msg whitelist[] = {
> > + { .svc = IMX_SC_RPC_SVC_MISC, .func =
> IMX_SC_MISC_FUNC_UNIQUE_ID },
> > + { .svc = IMX_SC_RPC_SVC_MISC, .func =
> > +IMX_SC_MISC_FUNC_GET_BUTTON_STATUS }, };
>
> Until now this low level IPC code didn't treat any svc/func specially and this
> seems good.
>
> The imx_scu_call_rpc function already has an have_resp argument and
> callers are responsible to fill it. Can't we deal with this by adding an
> additional err_ret flag passed by the caller?
Can you make it more detail? The have_resp is a bool, so where to add the flag?
The caller ONLY passes imx_sc_ipc, imx_sc_rpc_msg and have_resp, ONLY
imx_sc_ipc can add a flag, is it what you meant?
imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc, void *msg, bool have_resp)
>
> We can add wrapper functions to avoid tree-wide changes for all callers.
I agree, maybe we can add a new imx_scu_call_rpc function for those special APIs?
The new API will be ONLY for those APIs with response but without return value check,
then other callers will NOT be impacted, what do you think?
Thanks,
Anson
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RFCv4 0/7] interconnect: Add imx support via devfreq
From: Georgi Djakov @ 2019-09-26 0:51 UTC (permalink / raw)
To: Leonard Crestez, Rob Herring, Artur Świgoń,
Chanwoo Choi
Cc: Mark Rutland, devicetree@vger.kernel.org, Saravana Kannan,
linux-pm@vger.kernel.org, Stephen Boyd, Viresh Kumar,
Michael Turquette, Krzysztof Kozlowski, Kyungmin Park,
MyungJoo Ham, Alexandre Bailon, kernel@pengutronix.de,
Fabio Estevam, Shawn Guo, Aisheng Dong,
linux-arm-kernel@lists.infradead.org, dl-linux-imx
In-Reply-To: <VI1PR04MB7023ADED2210DFCB81E588CFEE870@VI1PR04MB7023.eurprd04.prod.outlook.com>
On 9/25/19 15:52, Leonard Crestez wrote:
> On 25.09.2019 05:38, Georgi Djakov wrote:
>> Hi Leonard,
>>
>> On 9/16/19 05:34, Leonard Crestez wrote:
>>> On 23.08.2019 17:37, Leonard Crestez wrote:
>>>> This series add imx support for interconnect via devfreq: the ICC
>>>> framework is used to aggregate requests from devices and then those are
>>>> converted to DEV_PM_QOS_MIN_FREQUENCY requests for devfreq.
>>>>
>>>> Since there is no single devicetree node that can represent the "interconnect"
>>>> new API is added to allow individual devfreq nodes to act as parsing proxies
>>>> all mapping to a single soc-level icc provider. This is still RFC
>>>> because of this
>>>
>>> Any comments? I made a lot of changes relative to previous versions,
>>> most of them solely to avoid adding a virtual node in DT bindings.
>>>
>>> The only current interconnect provider implementation is for qcom and it
>>> uses a firmware node as the provider node (with #interconnect-cells).
>>> However there is no obvious equivalent of that for imx and many other SOCs.
>>
>> Not sure if it will help, but have you seen the qcs404 interconnect driver?
>> There is also mt8183 interconnect provider driver on LKML.
>
> Yes, but only yesterday. The qcs404 driver involves multiple DT devices
> so it seems closer to imx.
>
> As far as I understand from reading qcs404 source:
>
> * There is no struct device representing the entire graph.
> * There are multiple NOCs and each registers itself as a separate
> interconnect provider.
> * Each NOC registers multiple icc_nodes of various sorts:
> * Device masters and slaves
> * Some nodes representing NoC ports?
Well, all nodes are representing ports.
> * Multiple internal nodes
> * There is single per-SOC master list of QNOCs in the qcs404 driver.
> * The QNOCs can reference each other between multiple providers.
> * Each NOC registers an icc_provider and a subset of the graph.
> * The multiple NoC inside a chip are distinguished by compat strings.
> This seems strange, aren't they really different instantiations of the
> same IP with small config changes?
No, they are different IPs - ahb, axi or custom based.
> This design is still quite odd, what would make sense to me is to
> register the "interconnect graph" once and then provide multiple
> "interconnect scalers" which handle the aggregated requests for certain
> specific nodes.
>
>>> On imx there are multiple pieces of scalable fabric which can be defined
>>> in DT as devfreq devices and it sort of makes sense to add
>>> #interconnect-cells to those. However when it comes to describing the
>>> SOC interconnect graph it's much more convenient to have a single
>>> per-SOC platform driver.
>>
>> Is all the NoC configuration done only by ATF? Are there any NoC related memory
>> mapped registers?
>
> Registers are memory-mapped and visible to the A-cores but should only
> be accessed through secure transactions. This means that configuration
> needs be done by ATF in EL3 (we don't support running linux in secure
> world on imx8m). There is no "remote processor" managing this on imx8m.
Can we create some noc DT node with it's memory mapped address and make
it an interconnect provider? Sounds to me like a more correct representation
of the hardware?
Other option would be to bless some PSCI DT node (for example) to be a
provider.
>
> On older imx6/7 chips we actually have two out-of-tree implementations
> of bus freq switching code: An older one in Linux (used when running in
> secure world) and a different one in optee for running Linux in
> non-secure world.
>
> NoC registers can be used to control some "transaction priority" bits
> but I don't want to expose that part right now.
This is very similar to some of the Qcom hardware.
> What determines bandwidth versus power consumption is the NoC clk rate
> and clocks are managed by Linux directly.
So you will need to describe these clocks in the interconnect provider
DT node like on qcs404.
> DVFS on the RAM controller (DDRC) is also important. That component is
> only a bus slave and frequency switching requires a complex sequence
> inside ATF.
Makes sense.
Thanks,
Georgi
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v8 4/6] PM / devfreq: Introduce get_freq_range helper
From: Chanwoo Choi @ 2019-09-26 1:06 UTC (permalink / raw)
To: Leonard Crestez, MyungJoo Ham, Kyungmin Park, Matthias Kaehlcke,
Viresh Kumar
Cc: Artur Świgoń, Abel Vesa, Saravana Kannan,
linux-pm@vger.kernel.org, dl-linux-imx, Krzysztof Kozlowski,
Lukasz Luba, Alexandre Bailon, Georgi Djakov,
linux-arm-kernel@lists.infradead.org, Jacky Bai
In-Reply-To: <VI1PR04MB70235A2C13E47029B3093D33EE870@VI1PR04MB7023.eurprd04.prod.outlook.com>
Hi,
On 19. 9. 26. 오전 5:55, Leonard Crestez wrote:
> On 25.09.2019 04:32, Chanwoo Choi wrote:
>> On 19. 9. 24. 오후 7:11, Leonard Crestez wrote:
>>> Moving handling of min/max freq to a single function and call it from
>>> update_devfreq and for printing min/max freq values in sysfs.
>>>
>>> This changes the behavior of out-of-range min_freq/max_freq: clamping
>>> is now done at evaluation time. This means that if an out-of-range
>>> constraint is imposed by sysfs and it later becomes valid then it will
>>> be enforced.
>>>
>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>>> ---
>>> drivers/devfreq/devfreq.c | 112 ++++++++++++++++++++++----------------
>>> 1 file changed, 64 insertions(+), 48 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index 4a878baa809e..eee403e70c84 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -96,10 +96,54 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
>>> dev_pm_opp_put(opp);
>>>
>>> return max_freq;
>>> }
>>>
>>> +/**
>>> + * get_freq_range() - Get the current freq range
>>> + * @devfreq: the devfreq instance
>>> + * @min_freq: the min frequency
>>> + * @max_freq: the max frequency
>>> + *
>>> + * This takes into consideration all constraints.
>>> + */
>>> +static void get_freq_range(struct devfreq *devfreq,
>>> + unsigned long *min_freq,
>>> + unsigned long *max_freq)
>>> +{
>>> + unsigned long *freq_table = devfreq->profile->freq_table;
>>> +
>>> + lockdep_assert_held(&devfreq->lock);
>>> +
>>> + /*
>>> + * Init min/max frequency from freq table.
>>
>> Init -> Initialize
>> min/max -> minimum/maximum
>>
>>> + * Drivers can initialize this in either ascending or descending order
>>
>> Drivers -> devfreq drivers
>>
>>> + * and devfreq core supports both.
>>> + */
>>
>>
>> In result, I prefer to change the comments as following:
>> /*
>> * Initialize the minimum/maximum frequency from freq_table.
>> * The devfreq drivers can initialize freq_table in either
>> * ascending or descending order and devfreq core supports both.
>> */
>
> OK
>
>>> + if (freq_table[0] < freq_table[devfreq->profile->max_state - 1]) {
>>> + *min_freq = freq_table[0];
>>> + *max_freq = freq_table[devfreq->profile->max_state - 1];
>>> + } else {
>>> + *min_freq = freq_table[devfreq->profile->max_state - 1];
>>> + *max_freq = freq_table[0];
>>> + }
>>> +
>>> + /* constraints from sysfs */
>>
>> 'constraints' -> Constraint because first verb have to be used
>> as the sigular verbs. Also, I think that have to enhance the comments
>> I prefer to use following comments:
>>
>> /* Constraint minimum/maximum frequency from user input via sysfs */
>>
>>
>>
>>> + *min_freq = max(*min_freq, devfreq->min_freq);
>>> + *max_freq = min(*max_freq, devfreq->max_freq);
>>> +
>>> + /* constraints from OPP interface */
>>
>> ditto. I prefer to use following comments:
>>
>> /* Constraint minimum/maximum frequency from OPP interface */
>>
>>
>>> + *min_freq = max(*min_freq, devfreq->scaling_min_freq);
>>> + /* scaling_max_freq can be zero on error */
>>
>> Please remove it.
>>
>>> + if (devfreq->scaling_max_freq)
>>
>> As I knew, devfreq->scaling_max_freq is never zero.
>> So, it is always true. This if statement is needed.
>
> It can happen if find_available_max_freq encounters an error when called
> from devfreq_notifier_call.
If you are wondering this case, I think that have to fix
the possible issue on there instead of this point.
>
> Maybe that should be separately fixed to set scaling_max_freq to a
> neutral value such as "ULONG_MAX" instead?
OK.
>
> BTW: the devfreq_notifier_call function returns -EINVAL on error but it
> should return one of the NOTIFY_OK/DONE/STOP values instead. The OPP
> framework ignores notifier results but (-EINVAL & NOTIFY_STOP) evaluates
> as true so other notifiers will be skipped unintentionally.
I agree. It is needed to fix the return value type.
>
>>> + *max_freq = min(*max_freq, devfreq->scaling_max_freq);
>>> +
>>> + /* max_freq takes precedence over min_freq */
>>
>> As I said, almost people know that min_freq have be under than max_freq.
>> Please remove it. And until finishing the discussion on mailing list,
>> please don't send the next version. If you just replied from my comment
>> and then wait my next comment, we can save the time without replying
>> the repetitive and same comment for same point.
>
> This series makes it possible to set a min_freq higher than max_freq
> (for example via PM QoS from various devices).
>
> It is not obvious that min_freq takes precedence over max_freq but the
> code is self-evident so I will remove the comment.
>
>>> + if (*min_freq > *max_freq)
>>> + *min_freq = *max_freq;
>>> +}
>>> +
>>> /**
>>> * devfreq_get_freq_level() - Lookup freq_table for the frequency
>>> * @devfreq: the devfreq instance
>>> * @freq: the target frequency
>>> */
>>> @@ -349,20 +393,11 @@ int update_devfreq(struct devfreq *devfreq)
>>>
>>> /* Reevaluate the proper frequency */
>>> err = devfreq->governor->get_target_freq(devfreq, &freq);
>>> if (err)
>>> return err;
>>> -
>>> - /*
>>> - * Adjust the frequency with user freq, QoS and available freq.
>>> - *
>>> - * List from the highest priority
>>> - * max_freq
>>> - * min_freq
>>> - */
>>> - max_freq = min(devfreq->scaling_max_freq, devfreq->max_freq);
>>> - min_freq = max(devfreq->scaling_min_freq, devfreq->min_freq);
>>> + get_freq_range(devfreq, &min_freq, &max_freq);
>>>
>>> if (freq < min_freq) {
>>> freq = min_freq;
>>> flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
>>> }
>>> @@ -1298,40 +1333,28 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
>>> ret = sscanf(buf, "%lu", &value);
>>> if (ret != 1)
>>> return -EINVAL;
>>>
>>> mutex_lock(&df->lock);
>>> -
>>> - if (value) {
>>> - if (value > df->max_freq) {
>>> - ret = -EINVAL;
>>> - goto unlock;
>>> - }
>>> - } else {
>>> - unsigned long *freq_table = df->profile->freq_table;
>>> -
>>> - /* Get minimum frequency according to sorting order */
>>> - if (freq_table[0] < freq_table[df->profile->max_state - 1])
>>> - value = freq_table[0];
>>> - else
>>> - value = freq_table[df->profile->max_state - 1];
>>> - }
>>> -
>>> df->min_freq = value;
>>> update_devfreq(df);
>>> - ret = count;
>>> -unlock:
>>> mutex_unlock(&df->lock);
>>> - return ret;
>>> +
>>> + return count;
>>> }
>>>
>>> static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
>>> char *buf)
>>> {
>>> struct devfreq *df = to_devfreq(dev);
>>> + unsigned long min_freq, max_freq;
>>> +
>>> + mutex_lock(&df->lock);
>>> + get_freq_range(df, &min_freq, &max_freq);
>>> + mutex_unlock(&df->lock);
>>>
>>> - return sprintf(buf, "%lu\n", max(df->scaling_min_freq, df->min_freq));
>>> + return sprintf(buf, "%lu\n", min_freq);
>>> }
>>>
>>> static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>>> const char *buf, size_t count)
>>> {
>>> @@ -1343,40 +1366,33 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>>> if (ret != 1)
>>> return -EINVAL;
>>>
>>> mutex_lock(&df->lock);
>>>
>>> - if (value) {
>>> - if (value < df->min_freq) {
>>> - ret = -EINVAL;
>>> - goto unlock;
>>> - }
>>> - } else {
>>> - unsigned long *freq_table = df->profile->freq_table;
>>> -
>>> - /* Get maximum frequency according to sorting order */
>>> - if (freq_table[0] < freq_table[df->profile->max_state - 1])
>>> - value = freq_table[df->profile->max_state - 1];
>>> - else
>>> - value = freq_table[0];
>>> - }
>>> + /* Interpret zero as "don't care" */
>>
>> Please remove it. Also, the detailed comment for user have to add
>> the documentation file.
>
> OK
>
>>
>>> + if (!value)
>>> + value = ULONG_MAX;
>>>
>>> df->max_freq = value;
>>> update_devfreq(df);
>>> - ret = count;
>>> -unlock:
>>> mutex_unlock(&df->lock);
>>> - return ret;
>>> +
>>> + return count;
>>> }
>>> static DEVICE_ATTR_RW(min_freq);
>>>
>>> static ssize_t max_freq_show(struct device *dev, struct device_attribute *attr,
>>> char *buf)
>>> {
>>> struct devfreq *df = to_devfreq(dev);
>>> + unsigned long min_freq, max_freq;
>>> +
>>> + mutex_lock(&df->lock);
>>> + get_freq_range(df, &min_freq, &max_freq);
>>> + mutex_unlock(&df->lock);
>>>
>>> - return sprintf(buf, "%lu\n", min(df->scaling_max_freq, df->max_freq));
>>> + return sprintf(buf, "%lu\n", max_freq);
>>> }
>>> static DEVICE_ATTR_RW(max_freq);
>>>
>>> static ssize_t available_frequencies_show(struct device *d,
>>> struct device_attribute *attr,
--
Best Regards,
Chanwoo Choi
Samsung Electronics
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v8 5/6] PM / devfreq: Add PM QoS support
From: Chanwoo Choi @ 2019-09-26 1:08 UTC (permalink / raw)
To: Leonard Crestez, Matthias Kaehlcke
Cc: Artur Świgoń, Abel Vesa, Saravana Kannan,
linux-pm@vger.kernel.org, Viresh Kumar, dl-linux-imx,
Krzysztof Kozlowski, Lukasz Luba, Kyungmin Park, MyungJoo Ham,
Alexandre Bailon, Georgi Djakov,
linux-arm-kernel@lists.infradead.org, Jacky Bai
In-Reply-To: <VI1PR04MB7023910B94E4920D329427C8EE870@VI1PR04MB7023.eurprd04.prod.outlook.com>
Hi,
On 19. 9. 26. 오전 4:40, Leonard Crestez wrote:
> On 25.09.2019 05:13, Chanwoo Choi wrote:
>> On 19. 9. 25. 오전 4:22, Leonard Crestez wrote:
>>> On 24.09.2019 22:11, Matthias Kaehlcke wrote:
>>>> On Tue, Sep 24, 2019 at 01:11:29PM +0300, Leonard Crestez wrote:
>>>>> Register notifiers with the PM QoS framework in order to respond to
>>>>> requests for DEV_PM_QOS_MIN_FREQUENCY and DEV_PM_QOS_MAX_FREQUENCY.
>>>>>
>>>>> No notifiers are added by this patch but PM QoS constraints can be
>>>>> imposed externally (for example from other devices).
>>>>>
>>>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>>>>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>>>>> ---
>>>>> drivers/devfreq/devfreq.c | 75 +++++++++++++++++++++++++++++++++++++++
>>>>> include/linux/devfreq.h | 5 +++
>>>>> 2 files changed, 80 insertions(+)
>>>>>
>>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>>> index eee403e70c84..784f3e40536a 100644
>>>>> --- a/drivers/devfreq/devfreq.c
>>>>> +++ b/drivers/devfreq/devfreq.c
>>>>> @@ -22,15 +22,18 @@
>>>>> #include <linux/platform_device.h>
>>>>> #include <linux/list.h>
>>>>> #include <linux/printk.h>
>>>>> #include <linux/hrtimer.h>
>>>>> #include <linux/of.h>
>>>>> +#include <linux/pm_qos.h>
>>>>> #include "governor.h"
>>>>>
>>>>> #define CREATE_TRACE_POINTS
>>>>> #include <trace/events/devfreq.h>
>>>>>
>>>>> +#define HZ_PER_KHZ 1000
>>>>> +
>>>>> static struct class *devfreq_class;
>>>>>
>>>>> /*
>>>>> * devfreq core provides delayed work based load monitoring helper
>>>>> * functions. Governors can use these or can implement their own
>>>>> @@ -109,10 +112,11 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
>>>>> static void get_freq_range(struct devfreq *devfreq,
>>>>> unsigned long *min_freq,
>>>>> unsigned long *max_freq)
>>>>> {
>>>>> unsigned long *freq_table = devfreq->profile->freq_table;
>>>>> + unsigned long qos_min_freq, qos_max_freq;
>>>>>
>>>>> lockdep_assert_held(&devfreq->lock);
>>>>>
>>>>> /*
>>>>> * Init min/max frequency from freq table.
>>>>> @@ -125,10 +129,18 @@ static void get_freq_range(struct devfreq *devfreq,
>>>>> } else {
>>>>> *min_freq = freq_table[devfreq->profile->max_state - 1];
>>>>> *max_freq = freq_table[0];
>>>>> }
>>>>>
>>>>> + /* constraints from PM QoS */
>>>>> + qos_min_freq = dev_pm_qos_read_value(devfreq->dev.parent,
>>>>> + DEV_PM_QOS_MIN_FREQUENCY);
>>>>> + qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent,
>>>>> + DEV_PM_QOS_MIN_FREQUENCY); >
>>>> This needs to be DEV_PM_QOS_MAX_FREQUENCY. I missed this in the earlier
>>>> reviews, but stumbled across it when testing.
>>>>
>>>> !Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>>>
>>> I broke it in v8 while processing comments. Thanks for catching it.
>>
>> Did you test this patchset with v8?
>> Maybe it is not working with this mistake.
>
> I ran some scripts which test that min_freq requests work as expected
> (using imx interconnect+devfreq). They don't touch max_freq.
We always have to test the code before contributing the patch.
Please test all cases for these patches.
>
> --
> Regards,
> Leonard
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v8 5/6] PM / devfreq: Add PM QoS support
From: Chanwoo Choi @ 2019-09-26 1:12 UTC (permalink / raw)
To: Leonard Crestez, Matthias Kaehlcke
Cc: Artur Świgoń, Abel Vesa, Saravana Kannan,
linux-pm@vger.kernel.org, Viresh Kumar, dl-linux-imx,
Krzysztof Kozlowski, Lukasz Luba, Kyungmin Park, MyungJoo Ham,
Alexandre Bailon, Georgi Djakov,
linux-arm-kernel@lists.infradead.org, Jacky Bai
In-Reply-To: <VI1PR04MB7023573BA3D5C5D521DB689CEE870@VI1PR04MB7023.eurprd04.prod.outlook.com>
On 19. 9. 26. 오전 6:18, Leonard Crestez wrote:
> On 25.09.2019 05:11, Chanwoo Choi wrote:
>> On 19. 9. 24. 오후 7:11, Leonard Crestez wrote:
>>> Register notifiers with the PM QoS framework in order to respond to
>>> requests for DEV_PM_QOS_MIN_FREQUENCY and DEV_PM_QOS_MAX_FREQUENCY.
>>>
>>> No notifiers are added by this patch but PM QoS constraints can be
>>> imposed externally (for example from other devices).
>>>
>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>>> ---
>>> drivers/devfreq/devfreq.c | 75 +++++++++++++++++++++++++++++++++++++++
>>> include/linux/devfreq.h | 5 +++
>>> 2 files changed, 80 insertions(+)
>>>
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index eee403e70c84..784f3e40536a 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -22,15 +22,18 @@
>>> #include <linux/platform_device.h>
>>> #include <linux/list.h>
>>> #include <linux/printk.h>
>>> #include <linux/hrtimer.h>
>>> #include <linux/of.h>
>>> +#include <linux/pm_qos.h>
>>> #include "governor.h"
>>>
>>> #define CREATE_TRACE_POINTS
>>> #include <trace/events/devfreq.h>
>>>
>>> +#define HZ_PER_KHZ 1000
>>> +
>>> static struct class *devfreq_class;
>>>
>>> /*
>>> * devfreq core provides delayed work based load monitoring helper
>>> * functions. Governors can use these or can implement their own
>>> @@ -109,10 +112,11 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
>>> static void get_freq_range(struct devfreq *devfreq,
>>> unsigned long *min_freq,
>>> unsigned long *max_freq)
>>> {
>>> unsigned long *freq_table = devfreq->profile->freq_table;
>>> + unsigned long qos_min_freq, qos_max_freq;
>>>
>>> lockdep_assert_held(&devfreq->lock);
>>>
>>> /*
>>> * Init min/max frequency from freq table.
>>> @@ -125,10 +129,18 @@ static void get_freq_range(struct devfreq *devfreq,
>>> } else {
>>> *min_freq = freq_table[devfreq->profile->max_state - 1];
>>> *max_freq = freq_table[0];
>>> }
>>>
>>> + /* constraints from PM QoS */
>>
>> As I commented on patch4,
>> 'constraints' -> 'Constraint' because first verb have to be used
>> as the sigular verbs.
>
> Already discussed for another patch; I will modify to "Apply constraints
> from PM QoS" instead.
I agree the new comment with 'Apply constraints ... '.
>
>> I prefer to use following comments:
>>
>> /* Constraint minimum/maximum frequency from PM QoS constraints */
>>
>>> + qos_min_freq = dev_pm_qos_read_value(devfreq->dev.parent,
>>> + DEV_PM_QOS_MIN_FREQUENCY);
>>> + qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent,
>>> + DEV_PM_QOS_MIN_FREQUENCY);
>>> + *min_freq = max(*min_freq, HZ_PER_KHZ * qos_min_freq);
>>> + *max_freq = min(*max_freq, HZ_PER_KHZ * qos_max_freq);
>>> +
>>> /* constraints from sysfs */
>>> *min_freq = max(*min_freq, devfreq->min_freq);
>>> *max_freq = min(*max_freq, devfreq->max_freq);
>>>
>>> /* constraints from OPP interface */
>>> @@ -606,10 +618,49 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
>>> mutex_unlock(&devfreq->lock);
>>>
>>> return ret;
>>> }
>>>
>>> +/**
>>> + * qos_notifier_call() - Common handler for QoS constraints.
>>> + * @devfreq: the devfreq instance.
>>> + */
>>> +static int qos_notifier_call(struct devfreq *devfreq)
>>> +{
>>> + int err;
>>> +
>>> + mutex_lock(&devfreq->lock);
>>> + err = update_devfreq(devfreq);
>>> + mutex_unlock(&devfreq->lock);
>>> + if (err)
>>> + dev_err(devfreq->dev.parent,
>>> + "failed to update frequency for PM QoS constraints (%d)\n",
>>
>> Is it not over 80 char?
>
> Yes but coding style explicitly forbids breaking strings.
>
>>> + err);
>>> +
>>> + return NOTIFY_OK;
>>> +}
>>> +
>>> +/**
>>> + * qos_min_notifier_call() - Callback for QoS min_freq changes.
>>> + * @nb: Should be devfreq->nb_min
>>> + */
>>> +static int qos_min_notifier_call(struct notifier_block *nb,
>>> + unsigned long val, void *ptr)
>>> +{
>>> + return qos_notifier_call(container_of(nb, struct devfreq, nb_min));
>>> +}
>>> +
>>> +/**
>>> + * qos_max_notifier_call() - Callback for QoS max_freq changes.
>>> + * @nb: Should be devfreq->nb_max
>>> + */
>>> +static int qos_max_notifier_call(struct notifier_block *nb,
>>> + unsigned long val, void *ptr)
>>> +{
>>> + return qos_notifier_call(container_of(nb, struct devfreq, nb_max));
>>> +}
>>> +
>>> /**
>>> * devfreq_dev_release() - Callback for struct device to release the device.
>>> * @dev: the devfreq device
>>> *
>>> * Remove devfreq from the list and release its resources.
>>> @@ -620,10 +671,15 @@ static void devfreq_dev_release(struct device *dev)
>>>
>>> mutex_lock(&devfreq_list_lock);
>>> list_del(&devfreq->node);
>>> mutex_unlock(&devfreq_list_lock);
>>>
>>> + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max,
>>> + DEV_PM_QOS_MAX_FREQUENCY);
>>> + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min,
>>> + DEV_PM_QOS_MIN_FREQUENCY);
>>> +
>>
>> Just print error with dev_err() without stopping the release step.
>>
>> I prefer to handle the return value if kernel API provides
>> the error code.
How about?
>>
>>> if (devfreq->profile->exit)
>>> devfreq->profile->exit(devfreq->dev.parent);
>>>
>>> kfree(devfreq->time_in_state);
>>> kfree(devfreq->trans_table);
>>> @@ -733,10 +789,28 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>> if (err) {
>>> put_device(&devfreq->dev);
>>> goto err_out;
>>> }
>>>
>>> + /*
>>> + * Register notifiers for updates to min/max_freq after device is
>>> + * initialized (and we can handle notifications) but before the
>>> + * governor is started (which should do an initial enforcement of
>>> + * constraints).
>>> + */
>>
>> My previous comment is not enough why I prefer to remove it. Sorry.
>> Actually, until now, the devfreq_add_device() don't have the detailed
>> comments because the line code is not too long. But, at the present time,
>> devfreq_add_device() is too long. It means that the detailed comment
>> are necessary.
>>
>> So, I'll add the detailed comment for each step of devfreq_add_device()
>> on separate patch to keep the same style. I'll send the patch to you
>> for the review.
>
> This is very likely to result in merge conflicts, maybe wait for my
> series to go in first?
I'll send the separate patch after finished the review of these patches.
So, if you agree, please remove this comment on this patch.
You can review the detailed comments on separate patch when I send.
>
>>> + devfreq->nb_min.notifier_call = qos_min_notifier_call;
>>> + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_min,
>>> + DEV_PM_QOS_MIN_FREQUENCY);
>>> + if (err)
>>> + goto err_devfreq;
>>> +
>>> + devfreq->nb_max.notifier_call = qos_max_notifier_call;
>>> + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_max,
>>> + DEV_PM_QOS_MAX_FREQUENCY);
>>> + if (err)
>>> + goto err_devfreq;
>>> +
>>> mutex_lock(&devfreq_list_lock);
>>>
>>> governor = try_then_request_governor(devfreq->governor_name);
>>> if (IS_ERR(governor)) {
>>> dev_err(dev, "%s: Unable to find governor for the device\n",
>>> @@ -760,10 +834,11 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>>
>>> return devfreq;
>>>
>>> err_init:
>>> mutex_unlock(&devfreq_list_lock);
>>> +err_devfreq:
>>> devfreq_remove_device(devfreq);
>>> return ERR_PTR(err);
>>>
>>> err_dev:
>>> /*
>>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>>> index c3cbc15fdf08..dac0dffeabb4 100644
>>> --- a/include/linux/devfreq.h
>>> +++ b/include/linux/devfreq.h
>>> @@ -134,10 +134,12 @@ struct devfreq_dev_profile {
>>> * @total_trans: Number of devfreq transitions
>>> * @trans_table: Statistics of devfreq transitions
>>> * @time_in_state: Statistics of devfreq states
>>> * @last_stat_updated: The last time stat updated
>>> * @transition_notifier_list: list head of DEVFREQ_TRANSITION_NOTIFIER notifier
>>> + * @nb_min: Notifier block for DEV_PM_QOS_MIN_FREQUENCY
>>> + * @nb_max: Notifier block for DEV_PM_QOS_MAX_FREQUENCY
>>> *
>>> * This structure stores the devfreq information for a give device.
>>> *
>>> * Note that when a governor accesses entries in struct devfreq in its
>>> * functions except for the context of callbacks defined in struct
>>> @@ -176,10 +178,13 @@ struct devfreq {
>>> unsigned int *trans_table;
>>> unsigned long *time_in_state;
>>> unsigned long last_stat_updated;
>>>
>>> struct srcu_notifier_head transition_notifier_list;
>>> +
>>> + struct notifier_block nb_min;
>>> + struct notifier_block nb_max;
>>> };
>>>
>>> struct devfreq_freqs {
>>> unsigned long old;
>>> unsigned long new;
>>>
>>
>>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v8 5/6] PM / devfreq: Add PM QoS support
From: Chanwoo Choi @ 2019-09-26 1:19 UTC (permalink / raw)
To: Leonard Crestez, Matthias Kaehlcke
Cc: Artur Świgoń, Abel Vesa, Saravana Kannan,
linux-pm@vger.kernel.org, Viresh Kumar, dl-linux-imx,
Krzysztof Kozlowski, Lukasz Luba, Kyungmin Park, MyungJoo Ham,
Alexandre Bailon, Georgi Djakov,
linux-arm-kernel@lists.infradead.org, Jacky Bai
In-Reply-To: <VI1PR04MB7023573BA3D5C5D521DB689CEE870@VI1PR04MB7023.eurprd04.prod.outlook.com>
On 19. 9. 26. 오전 6:18, Leonard Crestez wrote:
> On 25.09.2019 05:11, Chanwoo Choi wrote:
>> On 19. 9. 24. 오후 7:11, Leonard Crestez wrote:
>>> Register notifiers with the PM QoS framework in order to respond to
>>> requests for DEV_PM_QOS_MIN_FREQUENCY and DEV_PM_QOS_MAX_FREQUENCY.
>>>
>>> No notifiers are added by this patch but PM QoS constraints can be
>>> imposed externally (for example from other devices).
>>>
>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>>> ---
>>> drivers/devfreq/devfreq.c | 75 +++++++++++++++++++++++++++++++++++++++
>>> include/linux/devfreq.h | 5 +++
>>> 2 files changed, 80 insertions(+)
>>>
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index eee403e70c84..784f3e40536a 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -22,15 +22,18 @@
>>> #include <linux/platform_device.h>
>>> #include <linux/list.h>
>>> #include <linux/printk.h>
>>> #include <linux/hrtimer.h>
>>> #include <linux/of.h>
>>> +#include <linux/pm_qos.h>
>>> #include "governor.h"
>>>
>>> #define CREATE_TRACE_POINTS
>>> #include <trace/events/devfreq.h>
>>>
>>> +#define HZ_PER_KHZ 1000
>>> +
>>> static struct class *devfreq_class;
>>>
>>> /*
>>> * devfreq core provides delayed work based load monitoring helper
>>> * functions. Governors can use these or can implement their own
>>> @@ -109,10 +112,11 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
>>> static void get_freq_range(struct devfreq *devfreq,
>>> unsigned long *min_freq,
>>> unsigned long *max_freq)
>>> {
>>> unsigned long *freq_table = devfreq->profile->freq_table;
>>> + unsigned long qos_min_freq, qos_max_freq;
>>>
>>> lockdep_assert_held(&devfreq->lock);
>>>
>>> /*
>>> * Init min/max frequency from freq table.
>>> @@ -125,10 +129,18 @@ static void get_freq_range(struct devfreq *devfreq,
>>> } else {
>>> *min_freq = freq_table[devfreq->profile->max_state - 1];
>>> *max_freq = freq_table[0];
>>> }
>>>
>>> + /* constraints from PM QoS */
>>
>> As I commented on patch4,
>> 'constraints' -> 'Constraint' because first verb have to be used
>> as the sigular verbs.
>
> Already discussed for another patch; I will modify to "Apply constraints
> from PM QoS" instead.
>
>> I prefer to use following comments:
>>
>> /* Constraint minimum/maximum frequency from PM QoS constraints */
>>
>>> + qos_min_freq = dev_pm_qos_read_value(devfreq->dev.parent,
>>> + DEV_PM_QOS_MIN_FREQUENCY);
>>> + qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent,
>>> + DEV_PM_QOS_MIN_FREQUENCY);
>>> + *min_freq = max(*min_freq, HZ_PER_KHZ * qos_min_freq);
>>> + *max_freq = min(*max_freq, HZ_PER_KHZ * qos_max_freq);
>>> +
>>> /* constraints from sysfs */
>>> *min_freq = max(*min_freq, devfreq->min_freq);
>>> *max_freq = min(*max_freq, devfreq->max_freq);
>>>
>>> /* constraints from OPP interface */
>>> @@ -606,10 +618,49 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
>>> mutex_unlock(&devfreq->lock);
>>>
>>> return ret;
>>> }
>>>
>>> +/**
>>> + * qos_notifier_call() - Common handler for QoS constraints.
>>> + * @devfreq: the devfreq instance.
>>> + */
>>> +static int qos_notifier_call(struct devfreq *devfreq)
>>> +{
>>> + int err;
>>> +
>>> + mutex_lock(&devfreq->lock);
>>> + err = update_devfreq(devfreq);
>>> + mutex_unlock(&devfreq->lock);
>>> + if (err)
>>> + dev_err(devfreq->dev.parent,
>>> + "failed to update frequency for PM QoS constraints (%d)\n",
>>
>> Is it not over 80 char?
>
> Yes but coding style explicitly forbids breaking strings.
I want to make it within 80 char. How about following comment?
dev_err(devfreq->dev.parent,
"failed to update frequency from PM QoS (%d)\n",
>>> + err);
>>> +
>>> + return NOTIFY_OK;
>>> +}
>>> +
>>> +/**
>>> + * qos_min_notifier_call() - Callback for QoS min_freq changes.
>>> + * @nb: Should be devfreq->nb_min
>>> + */
>>> +static int qos_min_notifier_call(struct notifier_block *nb,
>>> + unsigned long val, void *ptr)
>>> +{
>>> + return qos_notifier_call(container_of(nb, struct devfreq, nb_min));
>>> +}
>>> +
>>> +/**
>>> + * qos_max_notifier_call() - Callback for QoS max_freq changes.
>>> + * @nb: Should be devfreq->nb_max
>>> + */
>>> +static int qos_max_notifier_call(struct notifier_block *nb,
>>> + unsigned long val, void *ptr)
>>> +{
>>> + return qos_notifier_call(container_of(nb, struct devfreq, nb_max));
>>> +}
>>> +
>>> /**
>>> * devfreq_dev_release() - Callback for struct device to release the device.
>>> * @dev: the devfreq device
>>> *
>>> * Remove devfreq from the list and release its resources.
>>> @@ -620,10 +671,15 @@ static void devfreq_dev_release(struct device *dev)
>>>
>>> mutex_lock(&devfreq_list_lock);
>>> list_del(&devfreq->node);
>>> mutex_unlock(&devfreq_list_lock);
>>>
>>> + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max,
>>> + DEV_PM_QOS_MAX_FREQUENCY);
>>> + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min,
>>> + DEV_PM_QOS_MIN_FREQUENCY);
>>> +
>>
>> Just print error with dev_err() without stopping the release step.
>>
>> I prefer to handle the return value if kernel API provides
>> the error code.
>>
>>> if (devfreq->profile->exit)
>>> devfreq->profile->exit(devfreq->dev.parent);
>>>
>>> kfree(devfreq->time_in_state);
>>> kfree(devfreq->trans_table);
>>> @@ -733,10 +789,28 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>> if (err) {
>>> put_device(&devfreq->dev);
>>> goto err_out;
>>> }
>>>
>>> + /*
>>> + * Register notifiers for updates to min/max_freq after device is
>>> + * initialized (and we can handle notifications) but before the
>>> + * governor is started (which should do an initial enforcement of
>>> + * constraints).
>>> + */
>>
>> My previous comment is not enough why I prefer to remove it. Sorry.
>> Actually, until now, the devfreq_add_device() don't have the detailed
>> comments because the line code is not too long. But, at the present time,
>> devfreq_add_device() is too long. It means that the detailed comment
>> are necessary.
>>
>> So, I'll add the detailed comment for each step of devfreq_add_device()
>> on separate patch to keep the same style. I'll send the patch to you
>> for the review.
>
> This is very likely to result in merge conflicts, maybe wait for my
> series to go in first?
>
>>> + devfreq->nb_min.notifier_call = qos_min_notifier_call;
>>> + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_min,
>>> + DEV_PM_QOS_MIN_FREQUENCY);
>>> + if (err)
>>> + goto err_devfreq;
>>> +
>>> + devfreq->nb_max.notifier_call = qos_max_notifier_call;
>>> + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_max,
>>> + DEV_PM_QOS_MAX_FREQUENCY);
>>> + if (err)
>>> + goto err_devfreq;
>>> +
>>> mutex_lock(&devfreq_list_lock);
>>>
>>> governor = try_then_request_governor(devfreq->governor_name);
>>> if (IS_ERR(governor)) {
>>> dev_err(dev, "%s: Unable to find governor for the device\n",
>>> @@ -760,10 +834,11 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>>
>>> return devfreq;
>>>
>>> err_init:
>>> mutex_unlock(&devfreq_list_lock);
>>> +err_devfreq:
>>> devfreq_remove_device(devfreq);
>>> return ERR_PTR(err);
>>>
>>> err_dev:
>>> /*
>>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>>> index c3cbc15fdf08..dac0dffeabb4 100644
>>> --- a/include/linux/devfreq.h
>>> +++ b/include/linux/devfreq.h
>>> @@ -134,10 +134,12 @@ struct devfreq_dev_profile {
>>> * @total_trans: Number of devfreq transitions
>>> * @trans_table: Statistics of devfreq transitions
>>> * @time_in_state: Statistics of devfreq states
>>> * @last_stat_updated: The last time stat updated
>>> * @transition_notifier_list: list head of DEVFREQ_TRANSITION_NOTIFIER notifier
>>> + * @nb_min: Notifier block for DEV_PM_QOS_MIN_FREQUENCY
>>> + * @nb_max: Notifier block for DEV_PM_QOS_MAX_FREQUENCY
>>> *
>>> * This structure stores the devfreq information for a give device.
>>> *
>>> * Note that when a governor accesses entries in struct devfreq in its
>>> * functions except for the context of callbacks defined in struct
>>> @@ -176,10 +178,13 @@ struct devfreq {
>>> unsigned int *trans_table;
>>> unsigned long *time_in_state;
>>> unsigned long last_stat_updated;
>>>
>>> struct srcu_notifier_head transition_notifier_list;
>>> +
>>> + struct notifier_block nb_min;
>>> + struct notifier_block nb_max;
>>> };
>>>
>>> struct devfreq_freqs {
>>> unsigned long old;
>>> unsigned long new;
>>>
>>
>>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ 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