[PATCH V10 4/5] of: unittest: Create overlay_common.dtsi and testcases_common.dtsi
In order to build-test the same unit-test files using fdtoverlay tool,
move the device nodes from the existing overlay_base.dts and
testcases_common.dts files to .dtsi counterparts. The .dts files now
include the new .dtsi files, resulting in exactly the same behavior as
earlier.
The .dtsi files can now be reused for compile time tests using
fdtoverlay (will be done by a later commit).
This is required because the base files passed to fdtoverlay tool
shouldn't be overlays themselves (i.e. shouldn't have the /plugin/;
tag).
Note that this commit also moves "testcase-device2" node to
testcases.dts from tests-interrupts.dtsi, as this node has a deliberate
error in it and is only relevant for runtime testing done with
unittest.c.
Signed-off-by: Viresh Kumar
---
drivers/of/unittest-data/overlay_base.dts | 90 +-
drivers/of/unittest-data/overlay_common.dtsi | 91 +++
drivers/of/unittest-data/testcases.dts| 23 ++---
.../of/unittest-data/testcases_common.dtsi| 19
.../of/unittest-data/tests-interrupts.dtsi| 11 +--
5 files changed, 128 insertions(+), 106 deletions(-)
create mode 100644 drivers/of/unittest-data/overlay_common.dtsi
create mode 100644 drivers/of/unittest-data/testcases_common.dtsi
diff --git a/drivers/of/unittest-data/overlay_base.dts
b/drivers/of/unittest-data/overlay_base.dts
index 99ab9d12d00b..ab9014589c5d 100644
--- a/drivers/of/unittest-data/overlay_base.dts
+++ b/drivers/of/unittest-data/overlay_base.dts
@@ -2,92 +2,4 @@
/dts-v1/;
/plugin/;
-/*
- * Base device tree that overlays will be applied against.
- *
- * Do not add any properties in node "/".
- * Do not add any nodes other than "/testcase-data-2" in node "/".
- * Do not add anything that would result in dtc creating node "/__fixups__".
- * dtc will create nodes "/__symbols__" and "/__local_fixups__".
- */
-
-/ {
- testcase-data-2 {
- #address-cells = <1>;
- #size-cells = <1>;
-
- electric_1: substation@100 {
- compatible = "ot,big-volts-control";
- reg = < 0x0100 0x100 >;
- status = "disabled";
-
- hvac_1: hvac-medium-1 {
- compatible = "ot,hvac-medium";
- heat-range = < 50 75 >;
- cool-range = < 60 80 >;
- };
-
- spin_ctrl_1: motor-1 {
- compatible = "ot,ferris-wheel-motor";
- spin = "clockwise";
- rpm_avail = < 50 >;
- };
-
- spin_ctrl_2: motor-8 {
- compatible = "ot,roller-coaster-motor";
- };
- };
-
- rides_1: fairway-1 {
- #address-cells = <1>;
- #size-cells = <1>;
- compatible = "ot,rides";
- status = "disabled";
- orientation = < 127 >;
-
- ride@100 {
- #address-cells = <1>;
- #size-cells = <1>;
- compatible = "ot,roller-coaster";
- reg = < 0x0100 0x100 >;
- hvac-provider = < _1 >;
- hvac-thermostat = < 29 > ;
- hvac-zones = < 14 >;
- hvac-zone-names = "operator";
- spin-controller = < _ctrl_2 5 _ctrl_2
7 >;
- spin-controller-names = "track_1", "track_2";
- queues = < 2 >;
-
- track@30 {
- reg = < 0x0030 0x10 >;
- };
-
- track@40 {
- reg = < 0x0040 0x10 >;
- };
-
- };
- };
-
- lights_1: lights@3 {
- compatible = "ot,work-lights";
- reg = < 0x0003 0x1000 >;
- status = "disabled";
- };
-
- lights_2: lights@4 {
- compatible = "ot,show-lights";
- reg = < 0x0004 0x1000 >;
- status = "disabled";
- rate = < 13 138 >;
-
[PATCH V10 3/5] kbuild: Allow .dtso format for overlay source files
Since the overlays dtb files are now named as .dtbo, there is a lot of interest in similarly naming the overlay source dts files as .dtso. This patch makes the necessary changes to allow .dtso format for overlay source files. Note that we still support generating .dtbo files from .dts files. This is required for the source files present in drivers/of/unittest-data/, because they can't be renamed to .dtso as they are used for some runtime testing as well. Reviewed-by: Geert Uytterhoeven Tested-by: Geert Uytterhoeven Signed-off-by: Viresh Kumar --- scripts/Makefile.lib | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index bc045a54a34e..59e86f67f9e0 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -339,7 +339,7 @@ $(obj)/%.dtb.S: $(obj)/%.dtb FORCE quiet_cmd_dtc = DTC $@ cmd_dtc = $(HOSTCC) -E $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) $< ; \ - $(DTC) -O $(patsubst .%,%,$(suffix $@)) -o $@ -b 0 \ + $(DTC) -I dts -O $(patsubst .%,%,$(suffix $@)) -o $@ -b 0 \ $(addprefix -i,$(dir $<) $(DTC_INCLUDE)) $(DTC_FLAGS) \ -d $(depfile).dtc.tmp $(dtc-tmp) ; \ cat $(depfile).pre.tmp $(depfile).dtc.tmp > $(depfile) @@ -347,9 +347,13 @@ cmd_dtc = $(HOSTCC) -E $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) $< ; $(obj)/%.dtb: $(src)/%.dts $(DTC) FORCE $(call if_changed_dep,dtc) +# Required for of unit-test files as they can't be renamed to .dtso $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE $(call if_changed_dep,dtc) +$(obj)/%.dtbo: $(src)/%.dtso $(DTC) FORCE + $(call if_changed_dep,dtc) + overlay-y := $(addprefix $(obj)/, $(overlay-y)) quiet_cmd_fdtoverlay = DTOVL $@ @@ -375,6 +379,9 @@ endef $(obj)/%.dt.yaml: $(src)/%.dts $(DTC) $(DT_TMP_SCHEMA) FORCE $(call if_changed_rule,dtc,yaml) +$(obj)/%.dt.yaml: $(src)/%.dtso $(DTC) $(DT_TMP_SCHEMA) FORCE + $(call if_changed_rule,dtc,yaml) + dtc-tmp = $(subst $(comma),_,$(dot-target).dts.tmp) # Bzip2 -- 2.25.0.rc1.19.g042ed3e048af
[PATCH V10 2/5] kbuild: Add generic rule to apply fdtoverlay
From: Rob Herring Add a generic rule to apply fdtoverlay in Makefile.lib, so every platform doesn't need to carry the complex rule. This also automatically adds "DTC_FLAGS_foo_base += -@" for all base files. The platform's Makefile only needs to have this now: foo-dtbs := foo_base.dtb foo_overlay1.dtbo foo_overlay2.dtbo dtb-y := foo.dtb We don't want to run schema checks on foo.dtb (as foo.dts doesn't exist) and the Makefile is updated accordingly. Signed-off-by: Rob Herring Co-developed-by: Viresh Kumar Signed-off-by: Viresh Kumar --- scripts/Makefile.lib | 26 -- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index a2658242d956..bc045a54a34e 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -75,11 +75,24 @@ always-y += $(userprogs-always-y) $(userprogs-always-m) # If CONFIG_OF_ALL_DTBS is enabled, all DT blobs are built dtb-$(CONFIG_OF_ALL_DTBS) += $(dtb-) +# List all dtbs to be generated by fdtoverlay +overlay-y := $(foreach m,$(dtb-y), $(if $(strip $($(m:.dtb=-dtbs))),$(m),)) + +# Generate symbols for the base files so overlays can be applied to them. +$(foreach m,$(overlay-y), $(eval DTC_FLAGS_$(basename $(firstword $($(m:.dtb=-dtbs += -@)) + +# Add base dtb and overlay dtbo +dtb-y += $(foreach m,$(overlay-y), $($(m:.dtb=-dtbs))) + always-y += $(dtb-y) ifneq ($(CHECK_DTBS),) -always-y += $(patsubst %.dtb,%.dt.yaml, $(dtb-y)) -always-y += $(patsubst %.dtbo,%.dt.yaml, $(dtb-y)) +# Don't run schema checks for dtbs created by fdtoverlay as they don't +# have corresponding dts files. +dt-yaml-y := $(filter-out $(overlay-y),$(dtb-y)) + +always-y += $(patsubst %.dtb,%.dt.yaml, $(dt-yaml-y)) +always-y += $(patsubst %.dtbo,%.dt.yaml, $(dt-yaml-y)) endif # Add subdir path @@ -337,6 +350,15 @@ $(obj)/%.dtb: $(src)/%.dts $(DTC) FORCE $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE $(call if_changed_dep,dtc) +overlay-y := $(addprefix $(obj)/, $(overlay-y)) + +quiet_cmd_fdtoverlay = DTOVL $@ + cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $(real-prereqs) + +$(overlay-y): FORCE + $(call if_changed,fdtoverlay) +$(call multi_depend, $(overlay-y), .dtb, -dtbs) + DT_CHECKER ?= dt-validate DT_BINDING_DIR := Documentation/devicetree/bindings # DT_TMP_SCHEMA may be overridden from Documentation/devicetree/bindings/Makefile -- 2.25.0.rc1.19.g042ed3e048af
[PATCH V10 1/5] kbuild: Simplify builds with CONFIG_OF_ALL_DTBS
We update 'extra-y' based on CONFIG_OF_ALL_DTBS three times. It would be far more straight forward if we rather update dtb-y to include all .dtb files if CONFIG_OF_ALL_DTBS is enabled. Acked-by: Masahiro Yamada Signed-off-by: Viresh Kumar --- scripts/Makefile.lib | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index eee59184de64..a2658242d956 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -73,14 +73,13 @@ always-y += $(userprogs-always-y) $(userprogs-always-m) # DTB # If CONFIG_OF_ALL_DTBS is enabled, all DT blobs are built +dtb-$(CONFIG_OF_ALL_DTBS) += $(dtb-) + always-y += $(dtb-y) -always-$(CONFIG_OF_ALL_DTBS) += $(dtb-) ifneq ($(CHECK_DTBS),) always-y += $(patsubst %.dtb,%.dt.yaml, $(dtb-y)) always-y += $(patsubst %.dtbo,%.dt.yaml, $(dtb-y)) -always-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtb,%.dt.yaml, $(dtb-)) -always-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtbo,%.dt.yaml, $(dtb-)) endif # Add subdir path -- 2.25.0.rc1.19.g042ed3e048af
[PATCH V10 0/5] dt: Add fdtoverlay rule and statically build unittest
Hi, This patchset adds a generic rule for applying overlays using fdtoverlay tool and then updates unittests to get built statically using the same. V9->V10: - Add a new patch to allow .dtso files. - Update 2/5 to be more efficient and also generate symbols for base files automatically. - No need to add lines like DTC_FLAGS_foo_base += -@ in patch 5/5. - Add Ack by Masahiro for 1/5. V8->V9: - Added some comment in patch 3/4 based on Frank's suggestions. V7->V8: - Patch 1 is new. - Platforms need to use dtb-y += foo.dtb instead of overlay-y += foo.dtb. - Use multi_depend instead of .SECONDEXPANSION. - Use dtb-y for unittest instead of overlay-y. - Rename the commented dtb filess in unittest Makefile as .dtbo. - Improved Makefile code (I am learning a lot every day :) V6->V7: - Dropped the first 4 patches, already merged. - Patch 1/3 is new, suggested by Rob and slightly modified by me. - Adapt Patch 3/3 to the new rule and name the overlay dtbs as .dtbo. -- Viresh Rob Herring (1): kbuild: Add generic rule to apply fdtoverlay Viresh Kumar (4): kbuild: Simplify builds with CONFIG_OF_ALL_DTBS kbuild: Allow .dtso format for overlay source files of: unittest: Create overlay_common.dtsi and testcases_common.dtsi of: unittest: Statically apply overlays using fdtoverlay drivers/of/unittest-data/Makefile | 48 ++ drivers/of/unittest-data/overlay_base.dts | 90 +- drivers/of/unittest-data/overlay_common.dtsi | 91 +++ drivers/of/unittest-data/static_base_1.dts| 4 + drivers/of/unittest-data/static_base_2.dts| 4 + drivers/of/unittest-data/testcases.dts| 23 ++--- .../of/unittest-data/testcases_common.dtsi| 19 .../of/unittest-data/tests-interrupts.dtsi| 11 +-- scripts/Makefile.lib | 40 ++-- 9 files changed, 218 insertions(+), 112 deletions(-) create mode 100644 drivers/of/unittest-data/overlay_common.dtsi create mode 100644 drivers/of/unittest-data/static_base_1.dts create mode 100644 drivers/of/unittest-data/static_base_2.dts create mode 100644 drivers/of/unittest-data/testcases_common.dtsi base-commit: a38fd8748464831584a19438cbb3082b5a2dab15 -- 2.25.0.rc1.19.g042ed3e048af
Re: [PATCH v6] i2c: virtio: add a virtio i2c frontend driver
On 05-03-21, 15:00, Jie Deng wrote:
> On 2021/3/5 11:09, Viresh Kumar wrote:
> > On 05-03-21, 09:46, Jie Deng wrote:
> > > On 2021/3/4 14:06, Viresh Kumar wrote:
> > > > > + mutex_lock(>i2c_lock);
> > > > I have never worked with i2c stuff earlier, but I don't think you need
> > > > a lock
> > > > here. The transactions seem to be serialized by the i2c-core by itself
> > > > (look at
> > > > i2c_transfer() in i2c-core-base.c), though there is another exported
> > > > version
> > > > __i2c_transfer() but the comment over it says the callers must take
> > > > adapter lock
> > > > before calling it.
> > > Lock is needed since no "lock_ops" is registered in this i2c_adapter.
> > drivers/i2c/i2c-core-base.c:
> >
> > static int i2c_register_adapter(struct i2c_adapter *adap)
> > {
> > ...
> >
> > if (!adap->lock_ops)
> > adap->lock_ops = _adapter_lock_ops;
> >
> > ...
> > }
> >
> > This should take care of it ?
>
>
> The problem is that you can't guarantee that adap->algo->master_xfer is only
> called
> from i2c_transfer. Any function who holds the adapter can call
> adap->algo->master_xfer
> directly. So I think it is safer to have a lock in virtio_i2c_xfer.
So I tried to look for such callers in the kernel.
$ git grep -l "\ > > > > +static struct i2c_adapter virtio_adapter = {
> > > > > + .owner = THIS_MODULE,
> > > > > + .name = "Virtio I2C Adapter",
> > > > > + .class = I2C_CLASS_DEPRECATED,
> > > > Why are we using something that is deprecated here ?
> > > Warn users that the adapter doesn't support classes anymore.
> > So this is the right thing to do? Or this is what we expect from new
> > drivers?
> > Sorry, I am just new to this stuff and so...
>
>
> https://patchwork.ozlabs.org/project/linux-i2c/patch/20170729121143.3980-1-...@the-dreams.de/
Frankly this confused me even further :)
The earlier comment in the code said:
"/* Warn users that adapter will stop using classes */"
so this looks more for existing drivers..
Then the commit message says this:
"Hopefully making clear that it is not needed for new drivers."
and comment says:
"/* Warn users that the adapter doesn't support classes anymore */"
Reading this it looks this is only required for existing adapters so they can
warn userspace and shouldn't be required for new drivers.
Am I reading it incorrectly ?
--
viresh
Re: [PATCH] opp: Invalidate current opp when draining the opp list
On 04-03-21, 15:07, Beata Michalska wrote:
> The current_opp when set, grabs additional reference on the opp,
> which is then supposed to be dropped upon releasing the opp table.
> Still both dev_pm_opp_remove_table and dev_pm_opp_remove_all_dynamic
> will completely drain the OPPs list, including dropping the additional
> reference on current_opp. This may lead to an attempt to access
> memory that has already been released. Make sure that while draining
> the list (in both dynamic and static cases) the current_opp gets
> actually invalidated.
>
> Fixes: 81c4d8a3c414 ("opp: Keep track of currently programmed OPP")
>
> Signed-off-by: Beata Michalska
> ---
> drivers/opp/core.c | 49 -
> 1 file changed, 32 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index c268938..10e65c4 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1502,10 +1502,39 @@ static struct dev_pm_opp *_opp_get_next(struct
> opp_table *opp_table,
> return opp;
> }
>
> -bool _opp_remove_all_static(struct opp_table *opp_table)
> +static int __opp_drain_list(struct opp_table *opp_table, bool dynamic)
> {
> struct dev_pm_opp *opp;
> + int count = 0;
> +
> + /*
> + * Can't remove the OPP from under the lock, debugfs removal needs to
> + * happen lock less to avoid circular dependency issues.
> + */
> + while ((opp = _opp_get_next(opp_table, dynamic))) {
> + /*
> + * The current_opp has extra hold on the ref count,
> + * still the draining here will result in all of them
> + * being dropped completely, so make
> + * sure no one will try to access the current_opp
> + * afterwords
> + */
> + if (opp_table->current_opp == opp &&
> + !(kref_read(>kref) - 1))
> + opp_table->current_opp = NULL;
Did you miss looking at:
static void _opp_table_kref_release(struct kref *kref)
{
...
if (opp_table->current_opp)
dev_pm_opp_put(opp_table->current_opp);
...
}
?
This is the place where the last reference to the current_opp is released and so
we shouldn't have any invalid access to it anywhere else.
Or am I missing some context here ?
--
viresh
Re: [PATCH v6] i2c: virtio: add a virtio i2c frontend driver
On 05-03-21, 09:46, Jie Deng wrote:
> On 2021/3/4 14:06, Viresh Kumar wrote:
> > depends on I2C as well ?
> No need that. The dependency of I2C is included in the Kconfig in its parent
> directory.
Sorry about that, I must have figured that out myself.
(Though a note on the way we reply to messages, please leave an empty line
before and after your messages, it gets difficult to find the inline replies
otherwise. )
> > > + if (!(req && req == [i])) {
> > I find this less readable compared to:
> > if (!req || req != [i]) {
>
> Different people may have different tastes.
>
> Please check Andy's comment in this link.
>
> https://lists.linuxfoundation.org/pipermail/virtualization/2020-September/049933.html
Heh, everyone wants you to do it differently :)
If we leave compilers optimizations aside (because it will generate the exact
same code for both the cases, I tested it as well to be doubly sure), The
original statement used in this patch has 3 conditional statements in it and the
way I suggested has only two.
Andy, thoughts ?
And anyway, this isn't biggest of my worries, just that I had to notice it
somehow :)
> > For all the above errors where you simply break out, you still need to free
> > the
> > memory for buf, right ?
> Will try to use reqs[i].buf = msgs[i].buf to avoid allocation.
I think it would be better to have all such deallocations done at a single
place, i.e. after the completion callback is finished.. Trying to solve this
everywhere is going to make this more messy.
> > > + mutex_lock(>i2c_lock);
> > I have never worked with i2c stuff earlier, but I don't think you need a
> > lock
> > here. The transactions seem to be serialized by the i2c-core by itself
> > (look at
> > i2c_transfer() in i2c-core-base.c), though there is another exported version
> > __i2c_transfer() but the comment over it says the callers must take adapter
> > lock
> > before calling it.
> Lock is needed since no "lock_ops" is registered in this i2c_adapter.
drivers/i2c/i2c-core-base.c:
static int i2c_register_adapter(struct i2c_adapter *adap)
{
...
if (!adap->lock_ops)
adap->lock_ops = _adapter_lock_ops;
...
}
This should take care of it ?
> >
> > > +
> > > + ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
> > > + if (ret == 0)
> > > + goto err_unlock_free;
> > > +
> > > + nr = ret;
> > > +
> > > + virtqueue_kick(vq);
> > > +
> > > + time_left = wait_for_completion_timeout(>completion, adap->timeout);
> > > + if (!time_left) {
> > > + dev_err(>dev, "virtio i2c backend timeout.\n");
> > > + ret = -ETIMEDOUT;
> > You need to free bufs of the requests here as well..
Just want to make sure you didn't miss this comment.
> > > +static struct i2c_adapter virtio_adapter = {
> > > + .owner = THIS_MODULE,
> > > + .name = "Virtio I2C Adapter",
> > > + .class = I2C_CLASS_DEPRECATED,
> > Why are we using something that is deprecated here ?
> Warn users that the adapter doesn't support classes anymore.
So this is the right thing to do? Or this is what we expect from new drivers?
Sorry, I am just new to this stuff and so...
> > > +struct virtio_i2c_out_hdr {
> > > + __le16 addr;
> > > + __le16 padding;
> > > + __le32 flags;
> > > +};
> > It might be worth setting __packed for the structures here, even when we
> > have
> > taken care of padding ourselves, for both the structures..
> Please check Michael's comment https://lkml.org/lkml/2020/9/3/339.
> I agreed to remove "__packed" .
When Michael commented the structure looked like this:
Actually it can be ignored as the compiler isn't going to add any padding by
itself in this case (since you already took care of it) as the structure will be
aligned to the size of the biggest element here. I generally consider it to be a
good coding-style to make sure we don't add any unwanted stuff in there by
mistake.
Anyway, we can see it in future if this is going to be required or not, if and
when we add new fields here.
--
viresh
Re: [PATCH v6] i2c: virtio: add a virtio i2c frontend driver
On 04-03-21, 09:59, Jie Deng wrote:
> Add an I2C bus driver for virtio para-virtualization.
>
> The controller can be emulated by the backend driver in
> any device model software by following the virtio protocol.
>
> The device specification can be found on
> https://lists.oasis-open.org/archives/virtio-comment/202101/msg8.html.
>
> By following the specification, people may implement different
> backend drivers to emulate different controllers according to
> their needs.
>
> Co-developed-by: Conghui Chen
> Signed-off-by: Conghui Chen
> Signed-off-by: Jie Deng
> ---
Please always supply version history, it makes it difficult to review otherwise.
> drivers/i2c/busses/Kconfig | 11 ++
> drivers/i2c/busses/Makefile | 3 +
> drivers/i2c/busses/i2c-virtio.c | 289
>
> include/uapi/linux/virtio_i2c.h | 42 ++
> include/uapi/linux/virtio_ids.h | 1 +
> 5 files changed, 346 insertions(+)
> create mode 100644 drivers/i2c/busses/i2c-virtio.c
> create mode 100644 include/uapi/linux/virtio_i2c.h
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 05ebf75..0860395 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -21,6 +21,17 @@ config I2C_ALI1535
> This driver can also be built as a module. If so, the module
> will be called i2c-ali1535.
>
> +config I2C_VIRTIO
> + tristate "Virtio I2C Adapter"
> + depends on VIRTIO
depends on I2C as well ?
> + help
> + If you say yes to this option, support will be included for the virtio
> + I2C adapter driver. The hardware can be emulated by any device model
> + software according to the virtio protocol.
> +
> + This driver can also be built as a module. If so, the module
> + will be called i2c-virtio.
> +
> config I2C_ALI1563
> tristate "ALI 1563"
> depends on PCI
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 615f35e..b88da08 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -6,6 +6,9 @@
> # ACPI drivers
> obj-$(CONFIG_I2C_SCMI) += i2c-scmi.o
>
> +# VIRTIO I2C host controller driver
> +obj-$(CONFIG_I2C_VIRTIO) += i2c-virtio.o
> +
Not that it is important but I would have added it towards the end instead of at
the top of the file..
> # PC SMBus host controller drivers
> obj-$(CONFIG_I2C_ALI1535)+= i2c-ali1535.o
> obj-$(CONFIG_I2C_ALI1563)+= i2c-ali1563.o
> diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
> new file mode 100644
> index 000..98c0fcc
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-virtio.c
> @@ -0,0 +1,289 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Virtio I2C Bus Driver
> + *
> + * The Virtio I2C Specification:
> + *
> https://raw.githubusercontent.com/oasis-tcs/virtio-spec/master/virtio-i2c.tex
> + *
> + * Copyright (c) 2021 Intel Corporation. All rights reserved.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
I don't think above two are required here..
> +#include
> +#include
> +#include
same here.
> +#include
same here.
> +
And this blank line as well, since all are standard linux headers.
> +#include
> +#include
> +
> +/**
> + * struct virtio_i2c - virtio I2C data
> + * @vdev: virtio device for this controller
> + * @completion: completion of virtio I2C message
> + * @adap: I2C adapter for this controller
> + * @i2c_lock: lock for virtqueue processing
> + * @vq: the virtio virtqueue for communication
> + */
> +struct virtio_i2c {
> + struct virtio_device *vdev;
> + struct completion completion;
> + struct i2c_adapter adap;
> + struct mutex i2c_lock;
i2c_ is redundant here. "lock" sounds good enough.
> + struct virtqueue *vq;
> +};
> +
> +/**
> + * struct virtio_i2c_req - the virtio I2C request structure
> + * @out_hdr: the OUT header of the virtio I2C message
> + * @buf: the buffer into which data is read, or from which it's written
> + * @in_hdr: the IN header of the virtio I2C message
> + */
> +struct virtio_i2c_req {
> + struct virtio_i2c_out_hdr out_hdr;
> + u8 *buf;
> + struct virtio_i2c_in_hdr in_hdr;
> +};
> +
> +static void virtio_i2c_msg_done(struct virtqueue *vq)
> +{
> + struct virtio_i2c *vi = vq->vdev->priv;
> +
> + complete(>completion);
> +}
> +
> +static int virtio_i2c_send_reqs(struct virtqueue *vq,
> + struct virtio_i2c_req *reqs,
> + struct i2c_msg *msgs, int nr)
> +{
> + struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr;
> + int i, outcnt, incnt, err = 0;
> + u8 *buf;
> +
> + for (i = 0; i < nr; i++) {
> + if (!msgs[i].len)
> + break;
> +
> + /* Only 7-bit mode supported for this moment. For the address
> format,
> + * Please check the Virtio I2C
Re: [PATCH V9 2/4] kbuild: Add generic rule to apply fdtoverlay
On 03-03-21, 11:49, Geert Uytterhoeven wrote: > Hi Viresh, > > On Wed, Mar 3, 2021 at 5:36 AM Viresh Kumar wrote: > > From: Rob Herring > > > > Add a generic rule to apply fdtoverlay in Makefile.lib, so every > > platform doesn't need to carry the complex rule. > > > > The platform's Makefile only needs to have this now: > > > > DTC_FLAGS_foo_base += -@ > > foo-dtbs := foo_base.dtb foo_overlay1.dtbo foo_overlay2.dtbo > > dtb-y := foo.dtb > > Is there a way to autogenerate the DTC_FLAGS_foo_base rule, based on > the foo-dtbs rule? Since the first entry in "foo-dtbs" is always going to be the only base file, maybe we can do that. -- viresh
Re: [PATCH V7 4/6] kbuild: Add support to build overlays (%.dtbo)
On 03-03-21, 11:44, Geert Uytterhoeven wrote: > Hi Viresh, > > On Wed, Mar 3, 2021 at 6:21 AM Viresh Kumar wrote: > > On 24-02-21, 19:32, Frank Rowand wrote: > > > I overlooked this and mistakenly thought that the move to .dtbo also > > > involved changing to .dtso. My bad. > > > > > > My favorite color here is to use .dtso for the source file that will > > > be compiled to create a .dtbo. > > > > > > Linus has already accepted patch 4/6 to 5.12-rc1, so changing to .dtso > > > will require another patch. > > > > Looks like this is what many people desire, lets do it and make it a > > standard even if it wasn't followed earlier. > > > > What about this ? > > Thanks, looks good to me, and works for me, so > Reviewed-by: Geert Uytterhoeven > Tested-by: Geert Uytterhoeven Thanks. > > --- a/scripts/Makefile.lib > > +++ b/scripts/Makefile.lib > > @@ -337,7 +337,7 @@ $(obj)/%.dtb.S: $(obj)/%.dtb FORCE > > > > quiet_cmd_dtc = DTC $@ > > cmd_dtc = $(HOSTCC) -E $(dtc_cpp_flags) -x assembler-with-cpp -o > > $(dtc-tmp) $< ; \ > > - $(DTC) -O $(patsubst .%,%,$(suffix $@)) -o $@ -b 0 \ > > + $(DTC) -I dts -O $(patsubst .%,%,$(suffix $@)) -o $@ -b 0 \ > > $(addprefix -i,$(dir $<) $(DTC_INCLUDE)) $(DTC_FLAGS) \ > > -d $(depfile).dtc.tmp $(dtc-tmp) ; \ > > cat $(depfile).pre.tmp $(depfile).dtc.tmp > $(depfile) > > @@ -348,6 +348,9 @@ $(obj)/%.dtb: $(src)/%.dts $(DTC) FORCE > > $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE > > $(call if_changed_dep,dtc) > > > > +$(obj)/%.dtbo: $(src)/%.dtso $(DTC) FORCE > > + $(call if_changed_dep,dtc) > > + > > overlay-y := $(addprefix $(obj)/, $(overlay-y)) > > > > quiet_cmd_fdtoverlay = DTOVL $@ > > @@ -373,6 +376,9 @@ endef > > $(obj)/%.dt.yaml: $(src)/%.dts $(DTC) $(DT_TMP_SCHEMA) FORCE > > $(call if_changed_rule,dtc,yaml) > > > > +$(obj)/%.dt.yaml: $(src)/%.dtso $(DTC) $(DT_TMP_SCHEMA) FORCE > > I'm wondering if "dt.yaml" should be changed to "dto.yaml" (here and in > the existing rule earlier in Makefile.lib), to avoid issues if both foo.dts > and > foo.dtso exist? Unlikely, but it might happen... I will let Rob answer that :) > > I had to keep the original line as is: > > > > $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE > > > > to support the unittest stuff as there are no dtso files there. There > > are few things we can do here: > > > > - Don't follow the dtso/dtbo convention for unittest, build files as > > dtb only and everything will continue to work I suppose as > > fdtoverlay won't complain. > > > > - Keep the above line in Makefile, this doesn't sound right, isn't it > > ? > > > > - Make .dts links for unittest file, maybe from the Makefile itself. > > > > - Something else ? > > Rename unittest .dts files to .dtso where applicable? They are used for some runtime tests, we are reusing them to do this testing as well, so renaming them is out of the question I believe. -- viresh
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On 03-03-21, 16:46, Jie Deng wrote: > This is not a problem. My original proposal was to mirror the struct > i2c_msg. > The code you looked at was based on that. > However, the virtio TC prefer not to mirror it. They have some concerns. > For example, there is a bit I2C_M_RD in i2c_msg.flag which has the same > meaning with > the R/W in virtio descriptor. This is a repetition which may cause problems. > So the virtio_i2c_out_hdr.flags is used to instead of i2c_msg.flags for > extension. So by default we don't support any of the existing flags except I2C_M_RD? #define I2C_M_TEN 0x0010 /* this is a ten bit chip address */ #define I2C_M_RD0x0001 /* read data, from slave to master */ #define I2C_M_STOP 0x8000 /* if I2C_FUNC_PROTOCOL_MANGLING */ #define I2C_M_NOSTART 0x4000 /* if I2C_FUNC_NOSTART */ #define I2C_M_REV_DIR_ADDR 0x2000 /* if I2C_FUNC_PROTOCOL_MANGLING */ #define I2C_M_IGNORE_NAK0x1000 /* if I2C_FUNC_PROTOCOL_MANGLING */ #define I2C_M_NO_RD_ACK 0x0800 /* if I2C_FUNC_PROTOCOL_MANGLING */ #define I2C_M_RECV_LEN 0x0400 /* length will be first received byte */ How do we work with clients who want to use such flags now ? -- viresh
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On 01-03-21, 14:41, Jie Deng wrote:
> diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
> +static int virtio_i2c_send_reqs(struct virtqueue *vq,
> + struct virtio_i2c_req *reqs,
> + struct i2c_msg *msgs, int nr)
> +{
> + struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr;
> + int i, outcnt, incnt, err = 0;
> + u8 *buf;
> +
> + for (i = 0; i < nr; i++) {
> + if (!msgs[i].len)
> + break;
> +
> + reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1);
> +
> + if (i != nr - 1)
> + reqs[i].out_hdr.flags |= VIRTIO_I2C_FLAGS_FAIL_NEXT;
> +
> + outcnt = incnt = 0;
> + sg_init_one(_hdr, [i].out_hdr,
> sizeof(reqs[i].out_hdr));
> + sgs[outcnt++] = _hdr;
> +
> + buf = kzalloc(msgs[i].len, GFP_KERNEL);
> + if (!buf)
> + break;
> +
> + if (msgs[i].flags & I2C_M_RD) {
> + reqs[i].read_buf = buf;
> + sg_init_one(_buf, reqs[i].read_buf, msgs[i].len);
> + sgs[outcnt + incnt++] = _buf;
> + } else {
> + reqs[i].write_buf = buf;
> + memcpy(reqs[i].write_buf, msgs[i].buf, msgs[i].len);
> + sg_init_one(_buf, reqs[i].write_buf, msgs[i].len);
> + sgs[outcnt++] = _buf;
> + }
> +
> + sg_init_one(_hdr, [i].in_hdr, sizeof(reqs[i].in_hdr));
> + sgs[outcnt + incnt++] = _hdr;
> +
> + err = virtqueue_add_sgs(vq, sgs, outcnt, incnt, [i],
> GFP_KERNEL);
> + if (err < 0) {
> + pr_err("failed to add msg[%d] to virtqueue.\n", i);
> + if (msgs[i].flags & I2C_M_RD) {
> + kfree(reqs[i].read_buf);
> + reqs[i].read_buf = NULL;
> + } else {
> + kfree(reqs[i].write_buf);
> + reqs[i].write_buf = NULL;
> + }
> + break;
> + }
> + }
> +
> + return i;
> +}
> diff --git a/include/uapi/linux/virtio_i2c.h b/include/uapi/linux/virtio_i2c.h
> +/**
> + * struct virtio_i2c_out_hdr - the virtio I2C message OUT header
> + * @addr: the controlled device address
> + * @padding: used to pad to full dword
> + * @flags: used for feature extensibility
> + */
> +struct virtio_i2c_out_hdr {
> + __le16 addr;
> + __le16 padding;
> + __le32 flags;
> +};
Both this code and the virtio spec (which is already merged) are
missing msgs[i].flags and they are never sent to backend. The only
flags available here are the ones defined by virtio spec and these are
not i2c flags.
I also looked at your i2c backend for acrn and it mistakenly copies
the hdr.flag, which is the virtio flag and not i2c flag.
https://github.com/projectacrn/acrn-hypervisor/blob/master/devicemodel/hw/pci/virtio/virtio_i2c.c#L539
I will send a fix for the specs if you agree that there is a problem
here.
what am I missing here ? This should have been caught in your testing
and so I feel I must be missing something.
--
viresh
Re: [PATCH V7 4/6] kbuild: Add support to build overlays (%.dtbo)
On 24-02-21, 19:32, Frank Rowand wrote: > I overlooked this and mistakenly thought that the move to .dtbo also > involved changing to .dtso. My bad. > > My favorite color here is to use .dtso for the source file that will > be compiled to create a .dtbo. > > Linus has already accepted patch 4/6 to 5.12-rc1, so changing to .dtso > will require another patch. Looks like this is what many people desire, lets do it and make it a standard even if it wasn't followed earlier. What about this ? diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index c430fbb36763..0dbedb61835f 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -337,7 +337,7 @@ $(obj)/%.dtb.S: $(obj)/%.dtb FORCE quiet_cmd_dtc = DTC $@ cmd_dtc = $(HOSTCC) -E $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) $< ; \ - $(DTC) -O $(patsubst .%,%,$(suffix $@)) -o $@ -b 0 \ + $(DTC) -I dts -O $(patsubst .%,%,$(suffix $@)) -o $@ -b 0 \ $(addprefix -i,$(dir $<) $(DTC_INCLUDE)) $(DTC_FLAGS) \ -d $(depfile).dtc.tmp $(dtc-tmp) ; \ cat $(depfile).pre.tmp $(depfile).dtc.tmp > $(depfile) @@ -348,6 +348,9 @@ $(obj)/%.dtb: $(src)/%.dts $(DTC) FORCE $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE $(call if_changed_dep,dtc) +$(obj)/%.dtbo: $(src)/%.dtso $(DTC) FORCE + $(call if_changed_dep,dtc) + overlay-y := $(addprefix $(obj)/, $(overlay-y)) quiet_cmd_fdtoverlay = DTOVL $@ @@ -373,6 +376,9 @@ endef $(obj)/%.dt.yaml: $(src)/%.dts $(DTC) $(DT_TMP_SCHEMA) FORCE $(call if_changed_rule,dtc,yaml) +$(obj)/%.dt.yaml: $(src)/%.dtso $(DTC) $(DT_TMP_SCHEMA) FORCE + $(call if_changed_rule,dtc,yaml) + dtc-tmp = $(subst $(comma),_,$(dot-target).dts.tmp) # Bzip2 -8<- I had to keep the original line as is: $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE to support the unittest stuff as there are no dtso files there. There are few things we can do here: - Don't follow the dtso/dtbo convention for unittest, build files as dtb only and everything will continue to work I suppose as fdtoverlay won't complain. - Keep the above line in Makefile, this doesn't sound right, isn't it ? - Make .dts links for unittest file, maybe from the Makefile itself. - Something else ? -- viresh
[PATCH V9 4/4] of: unittest: Statically apply overlays using fdtoverlay
Now that fdtoverlay is part of the kernel build, start using it to test the unitest overlays we have by applying them statically. Create two new base files static_base_1.dts and static_base_2.dts which includes other .dtsi files. Some unittest overlays deliberately contain errors that unittest checks for. These overlays will cause fdtoverlay to fail, and are thus not included for static builds. Signed-off-by: Viresh Kumar --- drivers/of/unittest-data/Makefile | 50 ++ drivers/of/unittest-data/static_base_1.dts | 4 ++ drivers/of/unittest-data/static_base_2.dts | 4 ++ 3 files changed, 58 insertions(+) create mode 100644 drivers/of/unittest-data/static_base_1.dts create mode 100644 drivers/of/unittest-data/static_base_2.dts diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile index 009f4045c8e4..f88b2f48f172 100644 --- a/drivers/of/unittest-data/Makefile +++ b/drivers/of/unittest-data/Makefile @@ -34,7 +34,57 @@ DTC_FLAGS_overlay += -@ DTC_FLAGS_overlay_bad_phandle += -@ DTC_FLAGS_overlay_bad_symbol += -@ DTC_FLAGS_overlay_base += -@ +DTC_FLAGS_static_base_1 += -@ +DTC_FLAGS_static_base_2 += -@ DTC_FLAGS_testcases += -@ # suppress warnings about intentional errors DTC_FLAGS_testcases += -Wno-interrupts_property + +# Apply overlays statically with fdtoverlay. This is a build time test that +# the overlays can be applied successfully by fdtoverlay. This does not +# guarantee that the overlays can be applied successfully at run time by +# unittest, but it provides a bit of build time test coverage for those +# who do not execute unittest. +# +# The overlays are applied on top of static_base_1.dtb and static_base_2.dtb to +# create static_test_1.dtb and static_test_2.dtb. If fdtoverlay detects an +# error than the kernel build will fail. static_test_1.dtb and +# static_test_2.dtb are not consumed by unittest. +# +# Some unittest overlays deliberately contain errors that unittest checks for. +# These overlays will cause fdtoverlay to fail, and are thus not included +# in the static test: +#overlay_bad_add_dup_node.dtbo \ +#overlay_bad_add_dup_prop.dtbo \ +#overlay_bad_phandle.dtbo \ +#overlay_bad_symbol.dtbo \ + +apply_static_overlay_1 := overlay_0.dtbo \ + overlay_1.dtbo \ + overlay_2.dtbo \ + overlay_3.dtbo \ + overlay_4.dtbo \ + overlay_5.dtbo \ + overlay_6.dtbo \ + overlay_7.dtbo \ + overlay_8.dtbo \ + overlay_9.dtbo \ + overlay_10.dtbo \ + overlay_11.dtbo \ + overlay_12.dtbo \ + overlay_13.dtbo \ + overlay_15.dtbo \ + overlay_gpio_01.dtbo \ + overlay_gpio_02a.dtbo \ + overlay_gpio_02b.dtbo \ + overlay_gpio_03.dtbo \ + overlay_gpio_04a.dtbo \ + overlay_gpio_04b.dtbo + +apply_static_overlay_2 := overlay.dtbo + +static_test_1-dtbs := static_base_1.dtb $(apply_static_overlay_1) +static_test_2-dtbs := static_base_2.dtb $(apply_static_overlay_2) + +dtb-$(CONFIG_OF_OVERLAY) += static_test_1.dtb static_test_2.dtb diff --git a/drivers/of/unittest-data/static_base_1.dts b/drivers/of/unittest-data/static_base_1.dts new file mode 100644 index ..10556cb3f01f --- /dev/null +++ b/drivers/of/unittest-data/static_base_1.dts @@ -0,0 +1,4 @@ +// SPDX-License-Identifier: GPL-2.0 +/dts-v1/; + +#include "testcases_common.dtsi" diff --git a/drivers/of/unittest-data/static_base_2.dts b/drivers/of/unittest-data/static_base_2.dts new file mode 100644 index ..b0ea9504d6f3 --- /dev/null +++ b/drivers/of/unittest-data/static_base_2.dts @@ -0,0 +1,4 @@ +// SPDX-License-Identifier: GPL-2.0 +/dts-v1/; + +#include "overlay_common.dtsi" -- 2.25.0.rc1.19.g042ed3e048af
[PATCH V9 3/4] of: unittest: Create overlay_common.dtsi and testcases_common.dtsi
In order to build-test the same unit-test files using fdtoverlay tool,
move the device nodes from the existing overlay_base.dts and
testcases_common.dts files to .dtsi counterparts. The .dts files now
include the new .dtsi files, resulting in exactly the same behavior as
earlier.
The .dtsi files can now be reused for compile time tests using
fdtoverlay (will be done by a later commit).
This is required because the base files passed to fdtoverlay tool
shouldn't be overlays themselves (i.e. shouldn't have the /plugin/;
tag).
Note that this commit also moves "testcase-device2" node to
testcases.dts from tests-interrupts.dtsi, as this node has a deliberate
error in it and is only relevant for runtime testing done with
unittest.c.
Signed-off-by: Viresh Kumar
---
drivers/of/unittest-data/overlay_base.dts | 90 +-
drivers/of/unittest-data/overlay_common.dtsi | 91 +++
drivers/of/unittest-data/testcases.dts| 23 ++---
.../of/unittest-data/testcases_common.dtsi| 19
.../of/unittest-data/tests-interrupts.dtsi| 11 +--
5 files changed, 128 insertions(+), 106 deletions(-)
create mode 100644 drivers/of/unittest-data/overlay_common.dtsi
create mode 100644 drivers/of/unittest-data/testcases_common.dtsi
diff --git a/drivers/of/unittest-data/overlay_base.dts
b/drivers/of/unittest-data/overlay_base.dts
index 99ab9d12d00b..ab9014589c5d 100644
--- a/drivers/of/unittest-data/overlay_base.dts
+++ b/drivers/of/unittest-data/overlay_base.dts
@@ -2,92 +2,4 @@
/dts-v1/;
/plugin/;
-/*
- * Base device tree that overlays will be applied against.
- *
- * Do not add any properties in node "/".
- * Do not add any nodes other than "/testcase-data-2" in node "/".
- * Do not add anything that would result in dtc creating node "/__fixups__".
- * dtc will create nodes "/__symbols__" and "/__local_fixups__".
- */
-
-/ {
- testcase-data-2 {
- #address-cells = <1>;
- #size-cells = <1>;
-
- electric_1: substation@100 {
- compatible = "ot,big-volts-control";
- reg = < 0x0100 0x100 >;
- status = "disabled";
-
- hvac_1: hvac-medium-1 {
- compatible = "ot,hvac-medium";
- heat-range = < 50 75 >;
- cool-range = < 60 80 >;
- };
-
- spin_ctrl_1: motor-1 {
- compatible = "ot,ferris-wheel-motor";
- spin = "clockwise";
- rpm_avail = < 50 >;
- };
-
- spin_ctrl_2: motor-8 {
- compatible = "ot,roller-coaster-motor";
- };
- };
-
- rides_1: fairway-1 {
- #address-cells = <1>;
- #size-cells = <1>;
- compatible = "ot,rides";
- status = "disabled";
- orientation = < 127 >;
-
- ride@100 {
- #address-cells = <1>;
- #size-cells = <1>;
- compatible = "ot,roller-coaster";
- reg = < 0x0100 0x100 >;
- hvac-provider = < _1 >;
- hvac-thermostat = < 29 > ;
- hvac-zones = < 14 >;
- hvac-zone-names = "operator";
- spin-controller = < _ctrl_2 5 _ctrl_2
7 >;
- spin-controller-names = "track_1", "track_2";
- queues = < 2 >;
-
- track@30 {
- reg = < 0x0030 0x10 >;
- };
-
- track@40 {
- reg = < 0x0040 0x10 >;
- };
-
- };
- };
-
- lights_1: lights@3 {
- compatible = "ot,work-lights";
- reg = < 0x0003 0x1000 >;
- status = "disabled";
- };
-
- lights_2: lights@4 {
- compatible = "ot,show-lights";
- reg = < 0x0004 0x1000 >;
- status = "disabled";
- rate = < 13 138 >;
-
[PATCH V9 1/4] kbuild: Simplify builds with CONFIG_OF_ALL_DTBS
We update 'extra-y' based on CONFIG_OF_ALL_DTBS three times. It would be far more straight forward if we rather update dtb-y to include all .dtb files if CONFIG_OF_ALL_DTBS is enabled. Signed-off-by: Viresh Kumar --- scripts/Makefile.lib | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index eee59184de64..a2658242d956 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -73,14 +73,13 @@ always-y += $(userprogs-always-y) $(userprogs-always-m) # DTB # If CONFIG_OF_ALL_DTBS is enabled, all DT blobs are built +dtb-$(CONFIG_OF_ALL_DTBS) += $(dtb-) + always-y += $(dtb-y) -always-$(CONFIG_OF_ALL_DTBS) += $(dtb-) ifneq ($(CHECK_DTBS),) always-y += $(patsubst %.dtb,%.dt.yaml, $(dtb-y)) always-y += $(patsubst %.dtbo,%.dt.yaml, $(dtb-y)) -always-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtb,%.dt.yaml, $(dtb-)) -always-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtbo,%.dt.yaml, $(dtb-)) endif # Add subdir path -- 2.25.0.rc1.19.g042ed3e048af
[PATCH V9 0/4] dt: Add fdtoverlay rule and statically build unittest
Hi, This patchset adds a generic rule for applying overlays using fdtoverlay tool and then updates unittests to get built statically using the same. V8->V9: - Added some comment in patch 3/4 based on Frank's suggestions. V7->V8: - Patch 1 is new. - Platforms need to use dtb-y += foo.dtb instead of overlay-y += foo.dtb. - Use multi_depend instead of .SECONDEXPANSION. - Use dtb-y for unittest instead of overlay-y. - Rename the commented dtb filess in unittest Makefile as .dtbo. - Improved Makefile code (I am learning a lot every day :) V6->V7: - Dropped the first 4 patches, already merged. - Patch 1/3 is new, suggested by Rob and slightly modified by me. - Adapt Patch 3/3 to the new rule and name the overlay dtbs as .dtbo. -- Viresh Rob Herring (1): kbuild: Add generic rule to apply fdtoverlay Viresh Kumar (3): kbuild: Simplify builds with CONFIG_OF_ALL_DTBS of: unittest: Create overlay_common.dtsi and testcases_common.dtsi of: unittest: Statically apply overlays using fdtoverlay drivers/of/unittest-data/Makefile | 50 ++ drivers/of/unittest-data/overlay_base.dts | 90 +- drivers/of/unittest-data/overlay_common.dtsi | 91 +++ drivers/of/unittest-data/static_base_1.dts| 4 + drivers/of/unittest-data/static_base_2.dts| 4 + drivers/of/unittest-data/testcases.dts| 23 ++--- .../of/unittest-data/testcases_common.dtsi| 19 .../of/unittest-data/tests-interrupts.dtsi| 11 +-- scripts/Makefile.lib | 29 +- 9 files changed, 210 insertions(+), 111 deletions(-) create mode 100644 drivers/of/unittest-data/overlay_common.dtsi create mode 100644 drivers/of/unittest-data/static_base_1.dts create mode 100644 drivers/of/unittest-data/static_base_2.dts create mode 100644 drivers/of/unittest-data/testcases_common.dtsi base-commit: fe07bfda2fb9cdef8a4d4008a409bb02f35f1bd8 -- 2.25.0.rc1.19.g042ed3e048af
[PATCH V9 2/4] kbuild: Add generic rule to apply fdtoverlay
From: Rob Herring Add a generic rule to apply fdtoverlay in Makefile.lib, so every platform doesn't need to carry the complex rule. The platform's Makefile only needs to have this now: DTC_FLAGS_foo_base += -@ foo-dtbs := foo_base.dtb foo_overlay1.dtbo foo_overlay2.dtbo dtb-y := foo.dtb We don't want to run schema checks on foo.dtb (as foo.dts doesn't exist) and the Makefile is updated accordingly. Signed-off-by: Rob Herring Co-developed-by: Viresh Kumar Signed-off-by: Viresh Kumar --- scripts/Makefile.lib | 24 ++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index a2658242d956..c430fbb36763 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -58,6 +58,10 @@ real-search = $(foreach m,$(1), $(if $(strip $(call suffix-search,$(m),$(2) -)), real-obj-y := $(call real-search, $(obj-y),-objs -y) real-obj-m := $(call real-search, $(obj-m),-objs -y -m) +# List all dtbs to be generated by fdtoverlay +overlay-y := $(foreach m,$(dtb-y), $(if $(strip $($(m:.dtb=-dtbs))),$(m),)) +overlay-$(CONFIG_OF_ALL_DTBS) += $(foreach m,$(dtb-), $(if $(strip $($(m:.dtb=-dtbs))),$(m),)) + always-y += $(always-m) # hostprogs-always-y += foo @@ -72,14 +76,21 @@ userprogs += $(userprogs-always-y) $(userprogs-always-m) always-y += $(userprogs-always-y) $(userprogs-always-m) # DTB +# Add base dtb and overlay dtbo +dtb-y += $(foreach m,$(overlay-y), $($(m:.dtb=-dtbs))) + # If CONFIG_OF_ALL_DTBS is enabled, all DT blobs are built dtb-$(CONFIG_OF_ALL_DTBS) += $(dtb-) always-y += $(dtb-y) ifneq ($(CHECK_DTBS),) -always-y += $(patsubst %.dtb,%.dt.yaml, $(dtb-y)) -always-y += $(patsubst %.dtbo,%.dt.yaml, $(dtb-y)) +# Don't run schema checks for dtbs created by fdtoverlay as they don't +# have corresponding dts files. +dt-yaml-y := $(filter-out $(overlay-y),$(dtb-y)) + +always-y += $(patsubst %.dtb,%.dt.yaml, $(dt-yaml-y)) +always-y += $(patsubst %.dtbo,%.dt.yaml, $(dt-yaml-y)) endif # Add subdir path @@ -337,6 +348,15 @@ $(obj)/%.dtb: $(src)/%.dts $(DTC) FORCE $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE $(call if_changed_dep,dtc) +overlay-y := $(addprefix $(obj)/, $(overlay-y)) + +quiet_cmd_fdtoverlay = DTOVL $@ + cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $(real-prereqs) + +$(overlay-y): FORCE + $(call if_changed,fdtoverlay) +$(call multi_depend, $(overlay-y), .dtb, -dtbs) + DT_CHECKER ?= dt-validate DT_BINDING_DIR := Documentation/devicetree/bindings # DT_TMP_SCHEMA may be overridden from Documentation/devicetree/bindings/Makefile -- 2.25.0.rc1.19.g042ed3e048af
Re: [PATCH 00/31] Introduce devm_pm_opp_* API
On 02-03-21, 16:40, Dmitry Osipenko wrote: > 20.01.2021 19:01, Dmitry Osipenko пишет: > > 01.01.2021 19:54, Yangtao Li пишет: > >> Hi, > >> > >> This patchset add devm_pm_opp_set_clkname, devm_pm_opp_put_clkname, > >> devm_pm_opp_set_regulators, devm_pm_opp_put_regulators, > >> devm_pm_opp_set_supported_hw, devm_pm_opp_of_add_table and > >> devm_pm_opp_register_notifier. > > > > Hello Yangtao, > > > > Thank you for your effort, looking forward to v2! > > Yangtao, could you please let me know what is the status of this series? > Will you be able to make a v2 anytime soon? Dmitry, if Yangtao doesn't reply back this week with a proposal, please go ahead and respin the patches yourself. Thanks. -- viresh
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On 02-03-21, 14:24, Jie Deng wrote: > Not for the full duplex. As Paolo explained in those links. > We defined a combined request called "write-read request" > > " > This is when a write is followed by a read: the master > starts off the transmission with a write, then sends a second START, > then continues with a read from the same address. I think above talks about the real I2C protocol at bus level ? > In theory there's no difference between one multi-segment transaction > and many single-segment transactions _in a single-master scenario_. > > However, it is a plausible configuration to have multiple guests sharing > an I2C host device as if they were multiple master. > > So the spec should provide a way at least to support for transactions with > 1 write and 1 read segment in one request to the same address. > " > From the perspective of specification design, it hopes to provide more > choices > while from the perspective of specific implementation, we can choose what we > need > as long as it does not violate the specification. That is fine, but what I was not able to understand was how do we get to know if we should expect both write and read bufs after the out header or only one of them ? I think I have understood that part now, we need to look at incnt and outcnt and make out what kind of transfer we need to do. - If outcnt == 1 and incnt == 2, then it is read operation. - If outcnt == 2 and incnt == 1, then it is write operation. - If outcnt == 2 and incnt == 2, then it is read-write operation. Anything else is invalid. Is my understanding correct here ? > Since the current Linux driver doesn't use this mechanism. I'm considering > to move > the "struct virtio_i2c_req" into the driver and use one "buf" instead. Linux can very much have its own definition of the structure and so I am not in favor of any such structure in the spec as well, it confuses people (like me) :). -- viresh
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On 02-03-21, 13:21, Jie Deng wrote:
>
> On 2021/3/2 12:42, Viresh Kumar wrote:
> > On 01-03-21, 14:41, Jie Deng wrote:
> > > +static int virtio_i2c_send_reqs(struct virtqueue *vq,
> > > + struct virtio_i2c_req *reqs,
> > > + struct i2c_msg *msgs, int nr)
> > > +{
> > > + struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr;
> > > + int i, outcnt, incnt, err = 0;
> > > + u8 *buf;
> > > +
> > > + for (i = 0; i < nr; i++) {
> > > + if (!msgs[i].len)
> > > + break;
> > > +
> > > + reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1);
> > And this won't work for 10 bit addressing right? Don't we support that
> > in kernel ?
> >
> > From Spec:
> >
> > \begin{tabular}{ |l||l|l|l|l|l|l|l|l|l|l|l|l|l|l|l|l| }
> > \hline
> > Bits & 15 & 14 & 13 & 12 & 11 & 10 & 9 & 8 & 7 & 6 & 5 & 4
> > & 3 & 2 & 1 & 0 \\
> > \hline
> > 7-bit address & 0 & 0 & 0 & 0 & 0 & 0 & 0 & 0 & A6 & A5 & A4 & A3
> > & A2 & A1 & A0 & 0 \\
> > \hline
> > 10-bit address & A7 & A6 & A5 & A4 & A3 & A2 & A1 & A0 & 1 & 1 & 1 & 1
> > & 0 & A9 & A8 & 0 \\
> > \hline
> > \end{tabular}
> Currently, to make things simple, this driver only supports 7 bit mode.
> It doesn't declare "I2C_FUNC_10BIT_ADDR" in the functionality.
> We may add in the future according to the need.
Okay, fair enough. Please add such details somewhere in the code so
people can understand it. You can very well add these at the top of
the file as well, in the general header.
--
viresh
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On 02-03-21, 13:06, Jie Deng wrote: > Yeah. Actually, the backend only needs "struct virtio_i2c_out_hdr out_hdr" > and "struct virtio_i2c_in_hdr in_hdr" for communication. So we only need to > keep > the first two in uapi and move "struct virtio_i2c_req" into the driver. > > But Jason wanted to include "struct virtio_i2c_req" in uapi. He explained in > this link > https://lists.linuxfoundation.org/pipermail/virtualization/2020-October/050222.html. > Do you agree with that explanation ? I am not sure I understood his reasoning well, but it doesn't make any sense to keep this in uapi header if this is never going to get transferred over the wire. Moreover, the struct virtio_i2c_req in spec is misleading to me and rather creates unnecessary confusion. There is no structure like this which ever get passed here, but rather there are multiple vq transactions which take place, one with just the out header, then one with buffer and finally one with in header. I am not sure what's the right way of documenting it or if this is a standard virtio world follows. -- viresh
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On 01-03-21, 14:41, Jie Deng wrote:
> +static int virtio_i2c_send_reqs(struct virtqueue *vq,
> + struct virtio_i2c_req *reqs,
> + struct i2c_msg *msgs, int nr)
> +{
> + struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr;
> + int i, outcnt, incnt, err = 0;
> + u8 *buf;
> +
> + for (i = 0; i < nr; i++) {
> + if (!msgs[i].len)
> + break;
> +
> + reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1);
And this won't work for 10 bit addressing right? Don't we support that
in kernel ?
>From Spec:
\begin{tabular}{ |l||l|l|l|l|l|l|l|l|l|l|l|l|l|l|l|l| }
\hline
Bits & 15 & 14 & 13 & 12 & 11 & 10 & 9 & 8 & 7 & 6 & 5 & 4 & 3
& 2 & 1 & 0 \\
\hline
7-bit address & 0 & 0 & 0 & 0 & 0 & 0 & 0 & 0 & A6 & A5 & A4 & A3 & A2
& A1 & A0 & 0 \\
\hline
10-bit address & A7 & A6 & A5 & A4 & A3 & A2 & A1 & A0 & 1 & 1 & 1 & 1 & 0
& A9 & A8 & 0 \\
\hline
\end{tabular}
--
viresh
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On 02-03-21, 09:31, Viresh Kumar wrote:
> On 01-03-21, 16:19, Arnd Bergmann wrote:
> > On Mon, Mar 1, 2021 at 7:41 AM Jie Deng wrote:
> >
> > > --- /dev/null
> > > +++ b/include/uapi/linux/virtio_i2c.h
> > > @@ -0,0 +1,56 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-or-later WITH Linux-syscall-note */
> > > +/*
> > > + * Definitions for virtio I2C Adpter
> > > + *
> > > + * Copyright (c) 2021 Intel Corporation. All rights reserved.
> > > + */
> > > +
> > > +#ifndef _UAPI_LINUX_VIRTIO_I2C_H
> > > +#define _UAPI_LINUX_VIRTIO_I2C_H
> >
> > Why is this a uapi header? Can't this all be moved into the driver
> > itself?
> >
> > > +/**
> > > + * struct virtio_i2c_req - the virtio I2C request structure
> > > + * @out_hdr: the OUT header of the virtio I2C message
> > > + * @write_buf: contains one I2C segment being written to the device
> > > + * @read_buf: contains one I2C segment being read from the device
> > > + * @in_hdr: the IN header of the virtio I2C message
> > > + */
> > > +struct virtio_i2c_req {
> > > + struct virtio_i2c_out_hdr out_hdr;
> > > + u8 *write_buf;
> > > + u8 *read_buf;
> > > + struct virtio_i2c_in_hdr in_hdr;
> > > +};
> >
> > In particular, this structure looks like it is only ever usable between
> > the transfer functions in the driver itself, it is shared with neither
> > user space nor the virtio host side.
>
> Why is it so ? Won't you expect hypervisors or userspace apps to use
> these ?
This comment applies only for the first two structures as the third
one is never exchanged over virtio.
--
viresh
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On 01-03-21, 16:19, Arnd Bergmann wrote:
> On Mon, Mar 1, 2021 at 7:41 AM Jie Deng wrote:
>
> > --- /dev/null
> > +++ b/include/uapi/linux/virtio_i2c.h
> > @@ -0,0 +1,56 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later WITH Linux-syscall-note */
> > +/*
> > + * Definitions for virtio I2C Adpter
> > + *
> > + * Copyright (c) 2021 Intel Corporation. All rights reserved.
> > + */
> > +
> > +#ifndef _UAPI_LINUX_VIRTIO_I2C_H
> > +#define _UAPI_LINUX_VIRTIO_I2C_H
>
> Why is this a uapi header? Can't this all be moved into the driver
> itself?
>
> > +/**
> > + * struct virtio_i2c_req - the virtio I2C request structure
> > + * @out_hdr: the OUT header of the virtio I2C message
> > + * @write_buf: contains one I2C segment being written to the device
> > + * @read_buf: contains one I2C segment being read from the device
> > + * @in_hdr: the IN header of the virtio I2C message
> > + */
> > +struct virtio_i2c_req {
> > + struct virtio_i2c_out_hdr out_hdr;
> > + u8 *write_buf;
> > + u8 *read_buf;
> > + struct virtio_i2c_in_hdr in_hdr;
> > +};
>
> In particular, this structure looks like it is only ever usable between
> the transfer functions in the driver itself, it is shared with neither
> user space nor the virtio host side.
Why is it so ? Won't you expect hypervisors or userspace apps to use
these ?
--
viresh
Re: [PATCH V8 0/4] dt: Add fdtoverlay rule and statically build unittest
On 01-03-21, 21:14, Frank Rowand wrote: > Hi Viresh, > > On 3/1/21 12:56 AM, Viresh Kumar wrote: > > On 12-02-21, 16:48, Viresh Kumar wrote: > >> Hi, > >> > >> This patchset adds a generic rule for applying overlays using fdtoverlay > >> tool and then updates unittests to get built statically using the same. > >> > >> V7->V8: > >> - Patch 1 is new. > >> - Platforms need to use dtb-y += foo.dtb instead of overlay-y += > >> foo.dtb. > >> - Use multi_depend instead of .SECONDEXPANSION. > >> - Use dtb-y for unittest instead of overlay-y. > >> - Rename the commented dtb filess in unittest Makefile as .dtbo. > >> - Improved Makefile code (I am learning a lot every day :) > > > > Ping! > > > > Please respin on 5.12-rc1, and pull in the change you said > you would make in response to my post v8 comment about the > v7 patches. Yes, I will do that. I must have been more explicit about the Ping I believe. It was more for Masahiro and Rob to see if the kbuild stuff (which is relatively new) makes sense or not before I respin this.. -- viresh
Re: [PATCH v3 2/2] powerpc: Remove remaining parts of oprofile
On 01-03-21, 12:09, Christophe Leroy wrote:
> Commit 9850b6c69356 ("arch: powerpc: Remove oprofile") removed
> oprofile.
>
> Remove all remaining parts of it.
>
> Signed-off-by: Christophe Leroy
> ---
> arch/powerpc/include/asm/cputable.h | 3 --
> arch/powerpc/kernel/cputable.c| 66 +--
> arch/powerpc/kernel/dt_cpu_ftrs.c | 4 --
> arch/powerpc/platforms/cell/spufs/spufs.h | 2 +-
> 4 files changed, 3 insertions(+), 72 deletions(-)
Great, I wasn't sure how the handle the cpu type stuff and so left it
for the right people to handle. :)
Acked-by: Viresh Kumar
--
viresh
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On 01-03-21, 16:46, Arnd Bergmann wrote: > But the driver does not support this at all: the sglist always has three > members as Viresh says: outhdr, msgbuf and inhdr. It then uses a > bounce buffer for the actual data transfer, and this always goes either > one way or the other. Yes and if the driver doesn't support the specification fully then it should say so in a comment at least and also fail in case someone tries a full duplex transfer.. -- viresh
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On 01-03-21, 14:10, Andy Shevchenko wrote:
> On Mon, Mar 01, 2021 at 02:09:25PM +0200, Andy Shevchenko wrote:
> > On Mon, Mar 01, 2021 at 05:24:41PM +0530, Viresh Kumar wrote:
> > > On 01-03-21, 14:41, Jie Deng wrote:
> > > > +/**
> > > > + * struct virtio_i2c_req - the virtio I2C request structure
> > > > + * @out_hdr: the OUT header of the virtio I2C message
> > > > + * @write_buf: contains one I2C segment being written to the device
> > > > + * @read_buf: contains one I2C segment being read from the device
> > > > + * @in_hdr: the IN header of the virtio I2C message
> > > > + */
> > > > +struct virtio_i2c_req {
> > > > + struct virtio_i2c_out_hdr out_hdr;
> > > > + u8 *write_buf;
> > > > + u8 *read_buf;
> > > > + struct virtio_i2c_in_hdr in_hdr;
> > > > +};
> > >
> > > I am not able to appreciate the use of write/read bufs here as we
> > > aren't trying to read/write data in the same transaction. Why do we
> > > have two bufs here as well as in specs ?
> >
> > I²C and SMBus support bidirectional transfers as well. I think two buffers
> > is
> > the right thing to do.
>
> Strictly speaking "half duplex".
Half duplex is what this driver implemented, i.e. only one side can
send a msg at once, we don't need two buffers for that for sure.
Though we need two buffers if we are ever going to support full
duplex.
--
viresh
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On 02-03-21, 10:21, Jie Deng wrote: > On 2021/3/1 19:54, Viresh Kumar wrote: > That's my original proposal. I used to mirror this interface with "struct > i2c_msg". > > But the design philosophy of virtio TC is that VIRTIO devices are not > specific to Linux > so the specs design should avoid the limitations of the current Linux driver > behavior. Right, I understand that. > We had some discussion about this. You may check these links to learn the > story. > https://lists.oasis-open.org/archives/virtio-comment/202010/msg00016.html > https://lists.oasis-open.org/archives/virtio-comment/202010/msg00033.html > https://lists.oasis-open.org/archives/virtio-comment/202011/msg00025.html So the thing is that we want to support full duplex mode, right ? How will that work protocol wise ? I mean how would we know if we are expecting both tx and rx buffers in a transfer ? -- viresh
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On 01-03-21, 14:41, Jie Deng wrote:
> +/**
> + * struct virtio_i2c_req - the virtio I2C request structure
> + * @out_hdr: the OUT header of the virtio I2C message
> + * @write_buf: contains one I2C segment being written to the device
> + * @read_buf: contains one I2C segment being read from the device
> + * @in_hdr: the IN header of the virtio I2C message
> + */
> +struct virtio_i2c_req {
> + struct virtio_i2c_out_hdr out_hdr;
> + u8 *write_buf;
> + u8 *read_buf;
> + struct virtio_i2c_in_hdr in_hdr;
> +};
I am not able to appreciate the use of write/read bufs here as we
aren't trying to read/write data in the same transaction. Why do we
have two bufs here as well as in specs ?
What about this on top of your patch ?
---
drivers/i2c/busses/i2c-virtio.c | 31 +++
include/uapi/linux/virtio_i2c.h | 3 +--
2 files changed, 12 insertions(+), 22 deletions(-)
diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
index 8c8bc9545418..e71ab1d2c83f 100644
--- a/drivers/i2c/busses/i2c-virtio.c
+++ b/drivers/i2c/busses/i2c-virtio.c
@@ -67,14 +67,13 @@ static int virtio_i2c_send_reqs(struct virtqueue *vq,
if (!buf)
break;
+ reqs[i].buf = buf;
+ sg_init_one(_buf, reqs[i].buf, msgs[i].len);
+
if (msgs[i].flags & I2C_M_RD) {
- reqs[i].read_buf = buf;
- sg_init_one(_buf, reqs[i].read_buf, msgs[i].len);
sgs[outcnt + incnt++] = _buf;
} else {
- reqs[i].write_buf = buf;
- memcpy(reqs[i].write_buf, msgs[i].buf, msgs[i].len);
- sg_init_one(_buf, reqs[i].write_buf, msgs[i].len);
+ memcpy(reqs[i].buf, msgs[i].buf, msgs[i].len);
sgs[outcnt++] = _buf;
}
@@ -84,13 +83,8 @@ static int virtio_i2c_send_reqs(struct virtqueue *vq,
err = virtqueue_add_sgs(vq, sgs, outcnt, incnt, [i],
GFP_KERNEL);
if (err < 0) {
pr_err("failed to add msg[%d] to virtqueue.\n", i);
- if (msgs[i].flags & I2C_M_RD) {
- kfree(reqs[i].read_buf);
- reqs[i].read_buf = NULL;
- } else {
- kfree(reqs[i].write_buf);
- reqs[i].write_buf = NULL;
- }
+ kfree(reqs[i].buf);
+ reqs[i].buf = NULL;
break;
}
}
@@ -118,14 +112,11 @@ static int virtio_i2c_complete_reqs(struct virtqueue *vq,
break;
}
- if (msgs[i].flags & I2C_M_RD) {
- memcpy(msgs[i].buf, req->read_buf, msgs[i].len);
- kfree(req->read_buf);
- req->read_buf = NULL;
- } else {
- kfree(req->write_buf);
- req->write_buf = NULL;
- }
+ if (msgs[i].flags & I2C_M_RD)
+ memcpy(msgs[i].buf, req->buf, msgs[i].len);
+
+ kfree(req->buf);
+ req->buf = NULL;
}
return i;
diff --git a/include/uapi/linux/virtio_i2c.h b/include/uapi/linux/virtio_i2c.h
index 92febf0c527e..61f0086ac75b 100644
--- a/include/uapi/linux/virtio_i2c.h
+++ b/include/uapi/linux/virtio_i2c.h
@@ -48,8 +48,7 @@ struct virtio_i2c_in_hdr {
*/
struct virtio_i2c_req {
struct virtio_i2c_out_hdr out_hdr;
- u8 *write_buf;
- u8 *read_buf;
+ u8 *buf;
struct virtio_i2c_in_hdr in_hdr;
};
--
viresh
Re: [PATCH V8 0/4] dt: Add fdtoverlay rule and statically build unittest
On 12-02-21, 16:48, Viresh Kumar wrote: > Hi, > > This patchset adds a generic rule for applying overlays using fdtoverlay > tool and then updates unittests to get built statically using the same. > > V7->V8: > - Patch 1 is new. > - Platforms need to use dtb-y += foo.dtb instead of overlay-y += > foo.dtb. > - Use multi_depend instead of .SECONDEXPANSION. > - Use dtb-y for unittest instead of overlay-y. > - Rename the commented dtb filess in unittest Makefile as .dtbo. > - Improved Makefile code (I am learning a lot every day :) Ping! -- viresh
[PATCH V5 1/2] topology: Allow multiple entities to provide sched_freq_tick() callback
This patch attempts to make it generic enough so other parts of the
kernel can also provide their own implementation of scale_freq_tick()
callback, which is called by the scheduler periodically to update the
per-cpu freq_scale variable.
The implementations now need to provide 'struct scale_freq_data' for the
CPUs for which they have hardware counters available, and a callback
gets registered for each possible CPU in a per-cpu variable.
The arch specific (or ARM AMU) counters are updated to adapt to this and
they take the highest priority if they are available, i.e. they will be
used instead of CPPC based counters for example.
The special code to rebuild the sched domains, in case invariance status
change for the system, is moved out of arm64 specific code and is added
to arch_topology.c.
Note that this also defines SCALE_FREQ_SOURCE_CPUFREQ but doesn't use it
and it is added to show that cpufreq is also acts as source of
information for FIE and will be used by default if no other counters are
supported for a platform.
Reviewed-by: Ionela Voinescu
Tested-by: Ionela Voinescu
Signed-off-by: Viresh Kumar
---
arch/arm64/include/asm/topology.h | 10 +--
arch/arm64/kernel/topology.c | 105 +++---
drivers/base/arch_topology.c | 85 ++--
include/linux/arch_topology.h | 14 +++-
4 files changed, 134 insertions(+), 80 deletions(-)
diff --git a/arch/arm64/include/asm/topology.h
b/arch/arm64/include/asm/topology.h
index 3b8dca4eb08d..ec2db3419c41 100644
--- a/arch/arm64/include/asm/topology.h
+++ b/arch/arm64/include/asm/topology.h
@@ -17,17 +17,9 @@ int pcibus_to_node(struct pci_bus *bus);
#include
void update_freq_counters_refs(void);
-void topology_scale_freq_tick(void);
-
-#ifdef CONFIG_ARM64_AMU_EXTN
-/*
- * Replace task scheduler's default counter-based
- * frequency-invariance scale factor setting.
- */
-#define arch_scale_freq_tick topology_scale_freq_tick
-#endif /* CONFIG_ARM64_AMU_EXTN */
/* Replace task scheduler's default frequency-invariant accounting */
+#define arch_scale_freq_tick topology_scale_freq_tick
#define arch_set_freq_scale topology_set_freq_scale
#define arch_scale_freq_capacity topology_get_freq_scale
#define arch_scale_freq_invariant topology_scale_freq_invariant
diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index e08a4126453a..47fca7376c93 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -199,12 +199,47 @@ static int freq_inv_set_max_ratio(int cpu, u64 max_rate,
u64 ref_rate)
return 0;
}
-static DEFINE_STATIC_KEY_FALSE(amu_fie_key);
-#define amu_freq_invariant() static_branch_unlikely(_fie_key)
+static void amu_scale_freq_tick(void)
+{
+ u64 prev_core_cnt, prev_const_cnt;
+ u64 core_cnt, const_cnt, scale;
+
+ prev_const_cnt = this_cpu_read(arch_const_cycles_prev);
+ prev_core_cnt = this_cpu_read(arch_core_cycles_prev);
+
+ update_freq_counters_refs();
+
+ const_cnt = this_cpu_read(arch_const_cycles_prev);
+ core_cnt = this_cpu_read(arch_core_cycles_prev);
+
+ if (unlikely(core_cnt <= prev_core_cnt ||
+const_cnt <= prev_const_cnt))
+ return;
+
+ /*
+* /\corearch_max_freq_scale
+* scale = --- *
+* /\const SCHED_CAPACITY_SCALE
+*
+* See validate_cpu_freq_invariance_counters() for details on
+* arch_max_freq_scale and the use of SCHED_CAPACITY_SHIFT.
+*/
+ scale = core_cnt - prev_core_cnt;
+ scale *= this_cpu_read(arch_max_freq_scale);
+ scale = div64_u64(scale >> SCHED_CAPACITY_SHIFT,
+ const_cnt - prev_const_cnt);
+
+ scale = min_t(unsigned long, scale, SCHED_CAPACITY_SCALE);
+ this_cpu_write(freq_scale, (unsigned long)scale);
+}
+
+static struct scale_freq_data amu_sfd = {
+ .source = SCALE_FREQ_SOURCE_ARCH,
+ .set_freq_scale = amu_scale_freq_tick,
+};
static void amu_fie_setup(const struct cpumask *cpus)
{
- bool invariant;
int cpu;
/* We are already set since the last insmod of cpufreq driver */
@@ -221,25 +256,10 @@ static void amu_fie_setup(const struct cpumask *cpus)
cpumask_or(amu_fie_cpus, amu_fie_cpus, cpus);
- invariant = topology_scale_freq_invariant();
-
- /* We aren't fully invariant yet */
- if (!invariant && !cpumask_equal(amu_fie_cpus, cpu_present_mask))
- return;
-
- static_branch_enable(_fie_key);
+ topology_set_scale_freq_source(_sfd, amu_fie_cpus);
pr_debug("CPUs[%*pbl]: counters will be used for FIE.",
cpumask_pr_args(cpus));
-
- /*
-* Task scheduler behavior depends on frequency invariance support,
-* either cpufreq or counter driven. If the support status changes as
-* a result of counter ini
[PATCH V5 2/2] cpufreq: CPPC: Add support for frequency invariance
The Frequency Invariance Engine (FIE) is providing a frequency scaling
correction factor that helps achieve more accurate load-tracking.
Normally, this scaling factor can be obtained directly with the help of
the cpufreq drivers as they know the exact frequency the hardware is
running at. But that isn't the case for CPPC cpufreq driver.
Another way of obtaining that is using the arch specific counter
support, which is already present in kernel, but that hardware is
optional for platforms.
This patch updates the CPPC driver to register itself with the topology
core to provide its own implementation (cppc_scale_freq_tick()) of
topology_scale_freq_tick() which gets called by the scheduler on every
tick. Note that the arch specific counters have higher priority than
CPPC counters, if available, though the CPPC driver doesn't need to have
any special handling for that.
On an invocation of cppc_scale_freq_tick(), we schedule an irq work
(since we reach here from hard-irq context), which then schedules a
normal work item and cppc_scale_freq_workfn() updates the per_cpu
freq_scale variable based on the counter updates since the last tick.
To allow platforms to disable frequency invariance support if they want,
this is all done under CONFIG_ACPI_CPPC_CPUFREQ_FIE, which is enabled by
default.
This also exports sched_setattr_nocheck() as the CPPC driver can be
built as a module.
Cc: Ionela Voinescu
Cc: linux-a...@vger.kernel.org
Signed-off-by: Viresh Kumar
---
drivers/cpufreq/Kconfig.arm| 9 ++
drivers/cpufreq/cppc_cpufreq.c | 244 +++--
include/linux/arch_topology.h | 1 +
kernel/sched/core.c| 1 +
4 files changed, 243 insertions(+), 12 deletions(-)
diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index e65e0a43be64..a3e2d6dfea70 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -19,6 +19,15 @@ config ACPI_CPPC_CPUFREQ
If in doubt, say N.
+config ACPI_CPPC_CPUFREQ_FIE
+ bool "Frequency Invariance support for CPPC cpufreq driver"
+ depends on ACPI_CPPC_CPUFREQ
+ default y
+ help
+ This enables frequency invariance support for CPPC cpufreq driver.
+
+ If in doubt, say N.
+
config ARM_ALLWINNER_SUN50I_CPUFREQ_NVMEM
tristate "Allwinner nvmem based SUN50I CPUFreq driver"
depends on ARCH_SUNXI
diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index 8a482c434ea6..c4580a37a1b1 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -10,14 +10,18 @@
#define pr_fmt(fmt)"CPPC Cpufreq:" fmt
+#include
#include
#include
#include
#include
#include
#include
+#include
+#include
#include
#include
+#include
#include
@@ -57,6 +61,203 @@ static struct cppc_workaround_oem_info wa_info[] = {
}
};
+#ifdef CONFIG_ACPI_CPPC_CPUFREQ_FIE
+
+/* Frequency invariance support */
+struct cppc_freq_invariance {
+ int cpu;
+ struct irq_work irq_work;
+ struct kthread_work work;
+ struct cppc_perf_fb_ctrs prev_perf_fb_ctrs;
+ struct cppc_cpudata *cpu_data;
+};
+
+static DEFINE_PER_CPU(struct cppc_freq_invariance, cppc_freq_inv);
+static struct kthread_worker *kworker_fie;
+
+static struct cpufreq_driver cppc_cpufreq_driver;
+static unsigned int hisi_cppc_cpufreq_get_rate(unsigned int cpu);
+static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
+struct cppc_perf_fb_ctrs fb_ctrs_t0,
+struct cppc_perf_fb_ctrs fb_ctrs_t1);
+
+/**
+ * cppc_scale_freq_workfn - CPPC freq_scale updater for frequency invariance
+ * @work: The work item.
+ *
+ * The CPPC driver register itself with the topology core to provide its own
+ * implementation (cppc_scale_freq_tick()) of topology_scale_freq_tick() which
+ * gets called by the scheduler on every tick.
+ *
+ * Note that the arch specific counters have higher priority than CPPC
counters,
+ * if available, though the CPPC driver doesn't need to have any special
+ * handling for that.
+ *
+ * On an invocation of cppc_scale_freq_tick(), we schedule an irq work (since
we
+ * reach here from hard-irq context), which then schedules a normal work item
+ * and cppc_scale_freq_workfn() updates the per_cpu freq_scale variable based
on
+ * the counter updates since the last tick.
+ */
+static void cppc_scale_freq_workfn(struct kthread_work *work)
+{
+ struct cppc_freq_invariance *cppc_fi;
+ struct cppc_perf_fb_ctrs fb_ctrs = {0};
+ struct cppc_cpudata *cpu_data;
+ unsigned long local_freq_scale;
+ u64 perf;
+
+ cppc_fi = container_of(work, struct cppc_freq_invariance, work);
+ cpu_data = cppc_fi->cpu_data;
+
+ if (cppc_get_perf_ctrs(cppc_fi->cpu, _ctrs)) {
+ pr_warn("%s: failed to read perf counters\n", __func__);
+ return;
[PATCH V5 0/2] cpufreq: cppc: Add support for frequency invariance
Hello, CPPC cpufreq driver is used for ARM servers and this patch series tries to provide counter-based frequency invariance support for them in the absence for architecture specific counters (like AMUs). This is tested by: - /me with some hacks on Hikey, as I didn't have access to the right hardware. - Vincent Guittot on ThunderX2, only initial testing done. - Ionela Voinescu on Juno R2, though she tested a previous version of this. This is based of 5.12-rc1. Changes since V4: - Move some code to policy specific initialization for cppc driver. - Initialize kthread specific stuff only once in cppc driver. - Added a kerneldoc comment in cppc driver and improved changelog as well. Changes since V3: - rebuild_sched_domains_energy() stuff moved from arm64 to drivers/base. - Added Reviewed/Tested-by Ionela for the first patch. - Remove unused max_freq field from structure in cppc driver. - s/cppc_f_i/cppc_freq_inv. - Fix an per-cpu access, there was a bug in earlier version. - Create a single kthread which can run on any CPU and takes care of work from all the CPUs. - Do the whole FIE thing under a new CONFIG option for cppc driver. - Few minor improvements. Changes since V2: - Not sending as an RFC anymore. - Several renames, reordering of code in 1/2 based on Ionela's comments. - Several rebase changes for 2/2. - The freq_scale calculations are optimized a bit. - Better overall commenting and commit logs. Changes since V1: - The interface for setting the callbacks is improved, so different parts looking to provide their callbacks don't need to think about each other. - Moved to per-cpu storage for storing the callback related data, AMU counters have higher priority with this. -- viresh Viresh Kumar (2): topology: Allow multiple entities to provide sched_freq_tick() callback cpufreq: CPPC: Add support for frequency invariance arch/arm64/include/asm/topology.h | 10 +- arch/arm64/kernel/topology.c | 105 + drivers/base/arch_topology.c | 85 ++- drivers/cpufreq/Kconfig.arm | 9 ++ drivers/cpufreq/cppc_cpufreq.c| 244 -- include/linux/arch_topology.h | 15 +- kernel/sched/core.c | 1 + 7 files changed, 377 insertions(+), 92 deletions(-) base-commit: fe07bfda2fb9cdef8a4d4008a409bb02f35f1bd8 -- 2.25.0.rc1.19.g042ed3e048af
Re: [PATCH] cpufreq: qcom-hw: fix dereferencing freed memory 'data'
On 28-02-21, 09:33, Shawn Guo wrote:
> Commit 67fc209b527d ("cpufreq: qcom-hw: drop devm_xxx() calls from
> init/exit hooks") introduces an issue of dereferencing freed memory
> 'data'. Fix it.
>
> Fixes: 67fc209b527d ("cpufreq: qcom-hw: drop devm_xxx() calls from init/exit
> hooks")
> Reported-by: kernel test robot
> Reported-by: Dan Carpenter
> Signed-off-by: Shawn Guo
> ---
> Viresh,
>
> The issue was introduced by v2 of "cpufreq: qcom-hw: drop devm_xxx()
> calls from init/exit hooks", which misses the conversion of 'data->base'
> in error path. Sorry!
>
> Shawn
>
> drivers/cpufreq/qcom-cpufreq-hw.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c
> b/drivers/cpufreq/qcom-cpufreq-hw.c
> index d3c23447b892..bee5d67a8227 100644
> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -374,7 +374,7 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy
> *policy)
> error:
> kfree(data);
> unmap_base:
> - iounmap(data->base);
> + iounmap(base);
> release_region:
> release_mem_region(res->start, resource_size(res));
> return ret;
Applied. Thanks.
--
viresh
Re: drivers/opp/of.c:959:12: warning: stack frame size of 2064 bytes in function '_of_add_table_indexed'
On 28-02-21, 02:11, kernel test robot wrote:
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> master
> head: 3fb6d0e00efc958d01c2f109c8453033a2d96796
> commit: 406e47652161d4f0d9bc4cd6237b36c51497ec75 opp: Create
> _of_add_table_indexed() to reduce code duplication
> date: 4 weeks ago
> config: powerpc64-randconfig-r002-20210227 (attached as .config)
> compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project
> 83bc7815c4235786111aa2abf7193292e4a602f5)
> reproduce (this is a W=1 build):
> wget
> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
> ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # install powerpc64 cross compiling tool for clang build
> # apt-get install binutils-powerpc64-linux-gnu
> #
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=406e47652161d4f0d9bc4cd6237b36c51497ec75
> git remote add linus
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> git fetch --no-tags linus master
> git checkout 406e47652161d4f0d9bc4cd6237b36c51497ec75
> # save the attached .config to linux build tree
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross
> ARCH=powerpc64
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot
>
> All warnings (new ones prefixed by >>):
>
> >> drivers/opp/of.c:959:12: warning: stack frame size of 2064 bytes in
> >> function '_of_add_table_indexed' [-Wframe-larger-than=]
>static int _of_add_table_indexed(struct device *dev, int index)
> ^
>1 warning generated.
>
>
> vim +/_of_add_table_indexed +959 drivers/opp/of.c
>
>958
> > 959static int _of_add_table_indexed(struct device *dev, int index)
>960{
>961struct opp_table *opp_table;
>962int ret, count;
Is this a false positive ?
--
viresh
Re: [PATCH v4] i2c: virtio: add a virtio i2c frontend driver
On 26-02-21, 10:46, Jie Deng wrote: > This v4 was the old version before the specification was acked by the virtio > tc. > > Following is the latest specification. > > https://raw.githubusercontent.com/oasis-tcs/virtio-spec/master/virtio-i2c.tex > > I will send the v5 since the host/guest ABI changes. Okay, now it makes some sense :) I am interested in this stuff, if possible please keep me Cc'd for following versions, thanks. -- viresh
Re: [PATCH v4] i2c: virtio: add a virtio i2c frontend driver
On 12-10-20, 09:55, Jie Deng wrote:
> Add an I2C bus driver for virtio para-virtualization.
>
> The controller can be emulated by the backend driver in
> any device model software by following the virtio protocol.
>
> This driver communicates with the backend driver through a
> virtio I2C message structure which includes following parts:
>
> - Header: i2c_msg addr, flags, len.
> - Data buffer: the pointer to the I2C msg data.
> - Status: the processing result from the backend.
>
> People may implement different backend drivers to emulate
> different controllers according to their needs. A backend
> example can be found in the device model of the open source
> project ACRN. For more information, please refer to
> https://projectacrn.org.
> diff --git a/include/uapi/linux/virtio_i2c.h b/include/uapi/linux/virtio_i2c.h
> new file mode 100644
> index 000..7413e45
> --- /dev/null
> +++ b/include/uapi/linux/virtio_i2c.h
> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later OR BSD-3-Clause */
> +/*
> + * Definitions for virtio I2C Adpter
> + *
> + * Copyright (c) 2020 Intel Corporation. All rights reserved.
> + */
> +
> +#ifndef _UAPI_LINUX_VIRTIO_I2C_H
> +#define _UAPI_LINUX_VIRTIO_I2C_H
> +
> +#include
> +#include
> +#include
> +
> +/**
> + * struct virtio_i2c_hdr - the virtio I2C message header structure
> + * @addr: i2c_msg addr, the slave address
> + * @flags: i2c_msg flags
> + * @len: i2c_msg len
> + */
> +struct virtio_i2c_hdr {
> + __le16 addr;
> + __le16 flags;
> + __le16 len;
> +};
Hi Jie,
I am a bit confused about the header and the format in which data is being
processed here. When I look at the specification present here:
https://lists.oasis-open.org/archives/virtio-comment/202009/msg00021.html
it talks about
struct virtio_i2c_out_hdr {
le16 addr;
le16 padding;
le32 flags;
};
struct virtio_i2c_in_hdr {
u8 status;
};
struct virtio_i2c_req {
struct virtio_i2c_out_hdr out_hdr;
u8 write_buf [];
u8 read_buf [];
struct virtio_i2c_in_hdr in_hdr;
};
while what we have above is completely different. What am I missing ?
--
viresh
Re: [PATCH V4 2/2] cpufreq: cppc: Add support for frequency invariance
On 22-02-21, 16:57, Rafael J. Wysocki wrote:
> On Mon, Feb 22, 2021 at 12:20 PM Viresh Kumar wrote:
> Even though the driver is located in drivers/cpufreq/ CPPC is part of
> ACPI and so a CC to linux-acpi is missing.
I just used get-maintainers, perhaps we should add an entry for this
in MAINTAINERS, will be orphan though..
> > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> > index e65e0a43be64..a3e2d6dfea70 100644
> > --- a/drivers/cpufreq/Kconfig.arm
> > +++ b/drivers/cpufreq/Kconfig.arm
> > @@ -19,6 +19,15 @@ config ACPI_CPPC_CPUFREQ
> >
> > If in doubt, say N.
> >
> > +config ACPI_CPPC_CPUFREQ_FIE
> > + bool "Frequency Invariance support for CPPC cpufreq driver"
> > + depends on ACPI_CPPC_CPUFREQ
>
> In theory, the CPPC cpufreq driver can be used on systems with
> nontrivial arch_freq_scale_tick() in which case the latter should be
> used I suppose.
>
> Would that actually happen if this option is enabled?
IIUC, you are saying that if this driver runs on x86 then we want
arch_freq_scale_tick() from arch/x86/kernel/smpboot.c to run instead
of this ? Yes that will happen because x86 doesn't enable
CONFIG_GENERIC_ARCH_TOPOLOGY and so this code will never trigger.
For other cases, like ARM AMU counters, the arch specific
implementation takes precedence to this.
> > +static void __init cppc_freq_invariance_init(void)
> > +{
> > + struct cppc_perf_fb_ctrs fb_ctrs = {0};
> > + struct cppc_freq_invariance *cppc_fi;
> > + struct sched_attr attr = {
> > + .size = sizeof(struct sched_attr),
> > + .sched_policy = SCHED_DEADLINE,
> > + .sched_nice = 0,
> > + .sched_priority = 0,
> > + /*
> > +* Fake (unused) bandwidth; workaround to "fix"
> > +* priority inheritance.
> > +*/
> > + .sched_runtime = 100,
> > + .sched_deadline = 1000,
> > + .sched_period = 1000,
> > + };
> > + int i, ret;
> > +
> > + if (cppc_cpufreq_driver.get == hisi_cppc_cpufreq_get_rate)
> > + return;
> > +
> > + kworker_fie = kthread_create_worker(0, "cppc_fie");
> > + if (IS_ERR(kworker_fie))
> > + return;
> > +
> > + for_each_possible_cpu(i) {
> > + cppc_fi = _cpu(cppc_freq_inv, i);
> > +
> > + /* A policy failed to initialize, abort */
> > + if (unlikely(!cppc_fi->cpu_data))
> > + return cppc_freq_invariance_exit();
> > +
> > + kthread_init_work(_fi->work, cppc_scale_freq_workfn);
> > + init_irq_work(_fi->irq_work, cppc_irq_work);
>
> What would be wrong with doing the above in
> cppc_freq_invariance_policy_init()? It looks like a better place to
> me.
Can move it there as well, I just kept policy specific stuff there as
ideally I wanted to do everything here.
> > + ret = sched_setattr_nocheck(kworker_fie->task, );
>
> And this needs to be done only once if I'm not mistaken.
Yes, I failed to fix this when I went to a single kworker.
--
viresh
Re: [PATCH v2] cpufreq: schedutil: Call sugov_update_next_freq() before check to fast_switch_enabled
On 24-02-21, 14:39, Yue Hu wrote:
> From: Yue Hu
>
> Note that sugov_update_next_freq() may return false, that means the
> caller sugov_fast_switch() will do nothing except fast switch check.
>
> Similarly, sugov_deferred_update() also has unnecessary operations
> of raw_spin_{lock,unlock} in sugov_update_single_freq() for that case.
>
> So, let's call sugov_update_next_freq() before the fast switch check
> to avoid unnecessary behaviors above. Accordingly, update interface
> definition to sugov_deferred_update() and remove sugov_fast_switch()
> since we will call cpufreq_driver_fast_switch() directly instead.
>
> Signed-off-by: Yue Hu
> ---
> v2: remove sugov_fast_switch() and call cpufreq_driver_fast_switch()
> directly instead, also update minor log message.
>
> kernel/sched/cpufreq_schedutil.c | 29 -
> 1 file changed, 12 insertions(+), 17 deletions(-)
Acked-by: Viresh Kumar
--
viresh
Re: [PATCH] cpufreq: schedutil: Call sugov_update_next_freq() before check to fast_switch_enabled
On 24-02-21, 13:42, Yue Hu wrote:
> From: Yue Hu
>
> Note that sugov_update_next_freq() may return false, that means the
> caller sugov_fast_switch() will do nothing except fast switch check.
>
> Similarly, sugov_deferred_update() also has unnecessary operations
> of raw_spin_{lock,unlock} in sugov_update_single_freq() for that case.
>
> So, let's call sugov_update_next_freq() before the fast switch check
> to avoid unnecessary behaviors above. Update the related interface
> definitions accordingly.
>
> Signed-off-by: Yue Hu
> ---
> kernel/sched/cpufreq_schedutil.c | 28 ++--
> 1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/kernel/sched/cpufreq_schedutil.c
> b/kernel/sched/cpufreq_schedutil.c
> index 41e498b..d23e5be 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -114,19 +114,13 @@ static bool sugov_update_next_freq(struct sugov_policy
> *sg_policy, u64 time,
> return true;
> }
>
> -static void sugov_fast_switch(struct sugov_policy *sg_policy, u64 time,
> - unsigned int next_freq)
> +static void sugov_fast_switch(struct sugov_policy *sg_policy, unsigned int
> next_freq)
> {
> - if (sugov_update_next_freq(sg_policy, time, next_freq))
> - cpufreq_driver_fast_switch(sg_policy->policy, next_freq);
> + cpufreq_driver_fast_switch(sg_policy->policy, next_freq);
I will call this directly instead, no need of the wrapper anymore.
> }
>
> -static void sugov_deferred_update(struct sugov_policy *sg_policy, u64 time,
> - unsigned int next_freq)
> +static void sugov_deferred_update(struct sugov_policy *sg_policy)
> {
> - if (!sugov_update_next_freq(sg_policy, time, next_freq))
> - return;
> -
> if (!sg_policy->work_in_progress) {
> sg_policy->work_in_progress = true;
> irq_work_queue(_policy->irq_work);
> @@ -368,16 +362,19 @@ static void sugov_update_single_freq(struct
> update_util_data *hook, u64 time,
> sg_policy->cached_raw_freq = cached_freq;
> }
>
> + if (!sugov_update_next_freq(sg_policy, time, next_f))
> + return;
> +
> /*
>* This code runs under rq->lock for the target CPU, so it won't run
>* concurrently on two different CPUs for the same target and it is not
>* necessary to acquire the lock in the fast switch case.
>*/
> if (sg_policy->policy->fast_switch_enabled) {
> - sugov_fast_switch(sg_policy, time, next_f);
> + sugov_fast_switch(sg_policy, next_f);
> } else {
> raw_spin_lock(_policy->update_lock);
> - sugov_deferred_update(sg_policy, time, next_f);
> + sugov_deferred_update(sg_policy);
> raw_spin_unlock(_policy->update_lock);
> }
> }
> @@ -456,12 +453,15 @@ static unsigned int sugov_next_freq_shared(struct
> sugov_cpu *sg_cpu, u64 time)
> if (sugov_should_update_freq(sg_policy, time)) {
> next_f = sugov_next_freq_shared(sg_cpu, time);
>
> + if (!sugov_update_next_freq(sg_policy, time, next_f))
> + goto unlock;
> +
> if (sg_policy->policy->fast_switch_enabled)
> - sugov_fast_switch(sg_policy, time, next_f);
> + sugov_fast_switch(sg_policy, next_f);
> else
> - sugov_deferred_update(sg_policy, time, next_f);
> + sugov_deferred_update(sg_policy);
> }
> -
> +unlock:
> raw_spin_unlock(_policy->update_lock);
> }
--
viresh
Re: [PATCH] scripts/dtc: Add missing fdtoverlay to gitignore
On 23-02-21, 15:12, Rob Herring wrote:
> Commit 0da6bcd9fcc0 ("scripts: dtc: Build fdtoverlay tool") enabled
> building fdtoverlay, but failed to add it to .gitignore.
>
> Also add a note to keep hostprogs in sync with .gitignore.
>
> Fixes: 0da6bcd9fcc0 ("scripts: dtc: Build fdtoverlay tool")
> Reported-by: Linus Torvalds
> Cc: Viresh Kumar
> Signed-off-by: Rob Herring
> ---
> Linus, please pick this up directly if you want. I'm going to be offline
> most of the next couple of days, will have a few other DT fixes to send
> you later this week.
>
> Rob
>
> scripts/dtc/.gitignore | 1 +
> scripts/dtc/Makefile | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/scripts/dtc/.gitignore b/scripts/dtc/.gitignore
> index b814e6076bdb..8a8b62bf3d3c 100644
> --- a/scripts/dtc/.gitignore
> +++ b/scripts/dtc/.gitignore
> @@ -1,2 +1,3 @@
> # SPDX-License-Identifier: GPL-2.0-only
> dtc
> +fdtoverlay
> diff --git a/scripts/dtc/Makefile b/scripts/dtc/Makefile
> index c8c21e0f2531..95aaf7431bff 100644
> --- a/scripts/dtc/Makefile
> +++ b/scripts/dtc/Makefile
> @@ -1,6 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0
> # scripts/dtc makefile
>
> +# *** Also keep .gitignore in sync when changing ***
> hostprogs-always-$(CONFIG_DTC) += dtc fdtoverlay
> hostprogs-always-$(CHECK_DT_BINDING) += dtc
Reviewed-by: Viresh Kumar
--
viresh
[PATCH V4 2/2] cpufreq: cppc: Add support for frequency invariance
The Frequency Invariance Engine (FIE) is providing a frequency scaling
correction factor that helps achieve more accurate load-tracking.
Normally, this scaling factor can be obtained directly with the help of
the cpufreq drivers as they know the exact frequency the hardware is
running at. But that isn't the case for CPPC cpufreq driver.
Another way of obtaining that is using the arch specific counter
support, which is already present in kernel, but that hardware is
optional for platforms.
This patch thus obtains this scaling factor using the existing logic
present in the cppc driver. Note that the arch specific counters have
higher priority than CPPC counters if available, though the CPPC driver
doesn't need to have any special handling for that.
To allow platforms to disable frequency invariance support if they want,
this is all done under CONFIG_ACPI_CPPC_CPUFREQ_FIE, which is enabled by
default.
This also exports sched_setattr_nocheck() as the CPPC driver can be
built as a module.
Cc: Ionela Voinescu
Signed-off-by: Viresh Kumar
---
drivers/cpufreq/Kconfig.arm| 9 ++
drivers/cpufreq/cppc_cpufreq.c | 223 +++--
include/linux/arch_topology.h | 1 +
kernel/sched/core.c| 1 +
4 files changed, 222 insertions(+), 12 deletions(-)
diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index e65e0a43be64..a3e2d6dfea70 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -19,6 +19,15 @@ config ACPI_CPPC_CPUFREQ
If in doubt, say N.
+config ACPI_CPPC_CPUFREQ_FIE
+ bool "Frequency Invariance support for CPPC cpufreq driver"
+ depends on ACPI_CPPC_CPUFREQ
+ default y
+ help
+ This enables frequency invariance support for CPPC cpufreq driver.
+
+ If in doubt, say N.
+
config ARM_ALLWINNER_SUN50I_CPUFREQ_NVMEM
tristate "Allwinner nvmem based SUN50I CPUFreq driver"
depends on ARCH_SUNXI
diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index 8a482c434ea6..fa1692db93c4 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -10,14 +10,18 @@
#define pr_fmt(fmt)"CPPC Cpufreq:" fmt
+#include
#include
#include
#include
#include
#include
#include
+#include
+#include
#include
#include
+#include
#include
@@ -57,6 +61,182 @@ static struct cppc_workaround_oem_info wa_info[] = {
}
};
+#ifdef CONFIG_ACPI_CPPC_CPUFREQ_FIE
+
+/* Frequency invariance support */
+struct cppc_freq_invariance {
+ int cpu;
+ struct irq_work irq_work;
+ struct kthread_work work;
+ struct cppc_perf_fb_ctrs prev_perf_fb_ctrs;
+ struct cppc_cpudata *cpu_data;
+};
+
+static DEFINE_PER_CPU(struct cppc_freq_invariance, cppc_freq_inv);
+static struct kthread_worker *kworker_fie;
+
+static struct cpufreq_driver cppc_cpufreq_driver;
+static unsigned int hisi_cppc_cpufreq_get_rate(unsigned int cpu);
+static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
+struct cppc_perf_fb_ctrs fb_ctrs_t0,
+struct cppc_perf_fb_ctrs fb_ctrs_t1);
+
+static void cppc_scale_freq_workfn(struct kthread_work *work)
+{
+ struct cppc_freq_invariance *cppc_fi;
+ struct cppc_perf_fb_ctrs fb_ctrs = {0};
+ struct cppc_cpudata *cpu_data;
+ unsigned long local_freq_scale;
+ u64 perf;
+
+ cppc_fi = container_of(work, struct cppc_freq_invariance, work);
+ cpu_data = cppc_fi->cpu_data;
+
+ if (cppc_get_perf_ctrs(cppc_fi->cpu, _ctrs)) {
+ pr_warn("%s: failed to read perf counters\n", __func__);
+ return;
+ }
+
+ cppc_fi->prev_perf_fb_ctrs = fb_ctrs;
+ perf = cppc_perf_from_fbctrs(cpu_data, cppc_fi->prev_perf_fb_ctrs,
+fb_ctrs);
+
+ perf <<= SCHED_CAPACITY_SHIFT;
+ local_freq_scale = div64_u64(perf, cpu_data->perf_caps.highest_perf);
+ if (WARN_ON(local_freq_scale > 1024))
+ local_freq_scale = 1024;
+
+ per_cpu(freq_scale, cppc_fi->cpu) = local_freq_scale;
+}
+
+static void cppc_irq_work(struct irq_work *irq_work)
+{
+ struct cppc_freq_invariance *cppc_fi;
+
+ cppc_fi = container_of(irq_work, struct cppc_freq_invariance, irq_work);
+ kthread_queue_work(kworker_fie, _fi->work);
+}
+
+static void cppc_scale_freq_tick(void)
+{
+ struct cppc_freq_invariance *cppc_fi = _cpu(cppc_freq_inv,
smp_processor_id());
+
+ /*
+* cppc_get_perf_ctrs() can potentially sleep, call that from the right
+* context.
+*/
+ irq_work_queue(_fi->irq_work);
+}
+
+static struct scale_freq_data cppc_sftd = {
+ .source = SCALE_FREQ_SOURCE_CPPC,
+ .set_freq_scale = cppc_scale_freq_tick,
+};
+
+static void cppc_freq_invariance_policy_init(struct cp
[PATCH V4 1/2] topology: Allow multiple entities to provide sched_freq_tick() callback
This patch attempts to make it generic enough so other parts of the
kernel can also provide their own implementation of scale_freq_tick()
callback, which is called by the scheduler periodically to update the
per-cpu freq_scale variable.
The implementations now need to provide 'struct scale_freq_data' for the
CPUs for which they have hardware counters available, and a callback
gets registered for each possible CPU in a per-cpu variable.
The arch specific (or ARM AMU) counters are updated to adapt to this and
they take the highest priority if they are available, i.e. they will be
used instead of CPPC based counters for example.
The special code to rebuild the sched domains, in case invariance status
change for the system, is moved out of arm64 specific code and is added
to arch_topology.c.
Note that this also defines SCALE_FREQ_SOURCE_CPUFREQ but doesn't use it
and it is added to show that cpufreq is also acts as source of
information for FIE and will be used by default if no other counters are
supported for a platform.
Reviewed-by: Ionela Voinescu
Tested-by: Ionela Voinescu
Signed-off-by: Viresh Kumar
---
arch/arm64/include/asm/topology.h | 10 +--
arch/arm64/kernel/topology.c | 105 +++---
drivers/base/arch_topology.c | 85 ++--
include/linux/arch_topology.h | 14 +++-
4 files changed, 134 insertions(+), 80 deletions(-)
diff --git a/arch/arm64/include/asm/topology.h
b/arch/arm64/include/asm/topology.h
index 3b8dca4eb08d..ec2db3419c41 100644
--- a/arch/arm64/include/asm/topology.h
+++ b/arch/arm64/include/asm/topology.h
@@ -17,17 +17,9 @@ int pcibus_to_node(struct pci_bus *bus);
#include
void update_freq_counters_refs(void);
-void topology_scale_freq_tick(void);
-
-#ifdef CONFIG_ARM64_AMU_EXTN
-/*
- * Replace task scheduler's default counter-based
- * frequency-invariance scale factor setting.
- */
-#define arch_scale_freq_tick topology_scale_freq_tick
-#endif /* CONFIG_ARM64_AMU_EXTN */
/* Replace task scheduler's default frequency-invariant accounting */
+#define arch_scale_freq_tick topology_scale_freq_tick
#define arch_set_freq_scale topology_set_freq_scale
#define arch_scale_freq_capacity topology_get_freq_scale
#define arch_scale_freq_invariant topology_scale_freq_invariant
diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index e08a4126453a..47fca7376c93 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -199,12 +199,47 @@ static int freq_inv_set_max_ratio(int cpu, u64 max_rate,
u64 ref_rate)
return 0;
}
-static DEFINE_STATIC_KEY_FALSE(amu_fie_key);
-#define amu_freq_invariant() static_branch_unlikely(_fie_key)
+static void amu_scale_freq_tick(void)
+{
+ u64 prev_core_cnt, prev_const_cnt;
+ u64 core_cnt, const_cnt, scale;
+
+ prev_const_cnt = this_cpu_read(arch_const_cycles_prev);
+ prev_core_cnt = this_cpu_read(arch_core_cycles_prev);
+
+ update_freq_counters_refs();
+
+ const_cnt = this_cpu_read(arch_const_cycles_prev);
+ core_cnt = this_cpu_read(arch_core_cycles_prev);
+
+ if (unlikely(core_cnt <= prev_core_cnt ||
+const_cnt <= prev_const_cnt))
+ return;
+
+ /*
+* /\corearch_max_freq_scale
+* scale = --- *
+* /\const SCHED_CAPACITY_SCALE
+*
+* See validate_cpu_freq_invariance_counters() for details on
+* arch_max_freq_scale and the use of SCHED_CAPACITY_SHIFT.
+*/
+ scale = core_cnt - prev_core_cnt;
+ scale *= this_cpu_read(arch_max_freq_scale);
+ scale = div64_u64(scale >> SCHED_CAPACITY_SHIFT,
+ const_cnt - prev_const_cnt);
+
+ scale = min_t(unsigned long, scale, SCHED_CAPACITY_SCALE);
+ this_cpu_write(freq_scale, (unsigned long)scale);
+}
+
+static struct scale_freq_data amu_sfd = {
+ .source = SCALE_FREQ_SOURCE_ARCH,
+ .set_freq_scale = amu_scale_freq_tick,
+};
static void amu_fie_setup(const struct cpumask *cpus)
{
- bool invariant;
int cpu;
/* We are already set since the last insmod of cpufreq driver */
@@ -221,25 +256,10 @@ static void amu_fie_setup(const struct cpumask *cpus)
cpumask_or(amu_fie_cpus, amu_fie_cpus, cpus);
- invariant = topology_scale_freq_invariant();
-
- /* We aren't fully invariant yet */
- if (!invariant && !cpumask_equal(amu_fie_cpus, cpu_present_mask))
- return;
-
- static_branch_enable(_fie_key);
+ topology_set_scale_freq_source(_sfd, amu_fie_cpus);
pr_debug("CPUs[%*pbl]: counters will be used for FIE.",
cpumask_pr_args(cpus));
-
- /*
-* Task scheduler behavior depends on frequency invariance support,
-* either cpufreq or counter driven. If the support status changes as
-* a result of counter ini
[PATCH V4 0/2] cpufreq: cppc: Add support for frequency invariance
Hello, CPPC cpufreq driver is used for ARM servers and this patch series tries to provide counter-based frequency invariance support for them in the absence for architecture specific counters (like AMUs). This is tested by: - /me with some hacks on Hikey, as I didn't have access to the right hardware. - Vincent Guittot on ThunderX2, only initial testing done so far. - Ionela Voinescu on Juno R2, though she tested the previous version of this. This is based of Linus's current master (so we will able to apply this on 5.12-rc1). Changes since V3: - rebuild_sched_domains_energy() stuff moved from arm64 to drivers/base. - Added Reviewed/Tested-by Ionela for the first patch. - Remove unused max_freq field from structure in cppc driver. - s/cppc_f_i/cppc_freq_inv. - Fix an per-cpu access, there was a bug in earlier version. - Create a single kthread which can run on any CPU and takes care of work from all the CPUs. - Do the whole FIE thing under a new CONFIG option for cppc driver. - Few minor improvements. Changes since V2: - Not sending as an RFC anymore. - Several renames, reordering of code in 1/2 based on Ionela's comments. - Several rebase changes for 2/2. - The freq_scale calculations are optimized a bit. - Better overall commenting and commit logs. Changes since V1: - The interface for setting the callbacks is improved, so different parts looking to provide their callbacks don't need to think about each other. - Moved to per-cpu storage for storing the callback related data, AMU counters have higher priority with this. -- viresh Viresh Kumar (2): topology: Allow multiple entities to provide sched_freq_tick() callback cpufreq: cppc: Add support for frequency invariance arch/arm64/include/asm/topology.h | 10 +- arch/arm64/kernel/topology.c | 105 ++ drivers/base/arch_topology.c | 85 +++- drivers/cpufreq/Kconfig.arm | 9 ++ drivers/cpufreq/cppc_cpufreq.c| 223 -- include/linux/arch_topology.h | 15 +- kernel/sched/core.c | 1 + 7 files changed, 356 insertions(+), 92 deletions(-) -- 2.25.0.rc1.19.g042ed3e048af
Re: [PATCH V3 2/2] cpufreq: cppc: Add support for frequency invariance
On 22-02-21, 11:00, Ionela Voinescu wrote:
> Hey,
>
> Some test results:
Nice, I haven't responded earlier as Vincent was also testing the
stuff out later last week and was planning to do it more this week.
> On Thursday 18 Feb 2021 at 16:35:38 (+), Ionela Voinescu wrote:
> [..]
> > > +static void __init cppc_freq_invariance_init(void)
> > > +{
> [..]
> > > +
> > > + ret = cppc_get_perf_ctrs(i, _ctrs);
> > > + if (!ret)
> > > + per_cpu(cppc_fi->prev_perf_fb_ctrs, i) = fb_ctrs;
> >
>
> After fixing this one:
> cppc_fi->prev_perf_fb_ctrs = fb_ctrs;
Yeah, I already fixed it and made several changes based on your
feedback.
> I got the following:
>
> Platform:
>
> - Juno R2 (CPUs [0-3] are littles, CPUs [4-5] are bigs)
> + PMU counters, used by CPPC through FFH
> + userspace/schedutil
>
>
> - Verifying that with userspace governor we see a correct change in
> scale factor:
>
> root@buildroot:~# dmesg | grep FIE
> [6.436770] AMU: CPUs[0-3]: AMU counters WON'T be used for FIE.
> [6.436962] AMU: CPUs[4-5]: AMU counters WON'T be used for FIE.
> [6.451510] CPPC:CPUs[0-5]: CPPC counters will be used for FIE.
>
> root@buildroot:~# echo 60 > policy4/scaling_setspeed
> [ 353.939495] CPU4: Invariance(cppc) scale: 512.
> [ 353.939497] CPU5: Invariance(cppc) scale: 512.
>
> root@buildroot:~# echo 120 > policy4/scaling_setspeed
> [ 372.683511] CPU5: Invariance(cppc) scale: 1024.
> [ 372.683518] CPU4: Invariance(cppc) scale: 1024.
>
> root@buildroot:~# echo 45 > policy0/scaling_setspeed
> [ 641.495513] CPU2: Invariance(cppc) scale: 485.
> [ 641.495514] CPU1: Invariance(cppc) scale: 485.
> [ 641.495517] CPU0: Invariance(cppc) scale: 485.
> [ 641.495542] CPU3: Invariance(cppc) scale: 485.
>
> root@buildroot:~# echo 95 > policy0/scaling_setspeed
> [ 852.015514] CPU2: Invariance(cppc) scale: 1024.
> [ 852.015514] CPU1: Invariance(cppc) scale: 1024.
> [ 852.015517] CPU0: Invariance(cppc) scale: 1024.
> [ 852.015541] CPU3: Invariance(cppc) scale: 1024.
Great.
> - I ran some benchmarks as well (perf, hackbench, dhrystone) on the same
>platform, using the userspace governor at fixed frequency, to evaluate
>the impact of the work we do or don't do on the tick.
>
>./perf bench sched pipe
>(10 iterations, higher is better, ops/s, comparisons with
>cpufreq-based FIE)
>
>cpufreq-based FIEAMU-based FIECPPC-based FIE
>
>39498.840984.7 38893.4
>std: 3.766%std: 4.461% std: 0.575%
> diff: 3.625% diff: -1.556%
>
>./hackbench -l 1000
>(10 iterations, lower is better, seconds, comparison with
>cpufreq-based FIE)
>
>cpufreq-based FIEAMU-based FIECPPC-based FIE
>
>6.4207 6.3386 6.7841
>std: 7.298%std: 2.252% std: 2.460%
> diff: -1.295%diff: 5.356%
>
>This shows a small regression for the CPPC-based FIE, but within the
>standard deviation.
>
>I ran some dhrystone benchmarks (./dhrystone -t 2/34/5/6/ -l 5000) as
>well with schedutil governor to understand if an increase in accuracy
>with the AMU/CPPC counters makes a difference. Given the
>characteristics of the platform it's no surprise that the results
>were very similar between the three cases, so I won't bore you with
>the numbers.
Nice, I have much more confidence on this stuff now :)
Thanks a lot Ionela, I will resend the series again today then.
--
viresh
[PATCH] mailbox: arm_mhuv2: Skip calling kfree() with invalid pointer
It is possible that 'data' passed to kfree() is set to a error value
instead of allocated space. Make sure it doesn't get called with invalid
pointer.
Fixes: 5a6338cce9f4 ("mailbox: arm_mhuv2: Add driver")
Cc: v5.11 # v5.11
Reported-by: kernel test robot
Reported-by: Dan Carpenter
Signed-off-by: Viresh Kumar
---
drivers/mailbox/arm_mhuv2.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/mailbox/arm_mhuv2.c b/drivers/mailbox/arm_mhuv2.c
index cdfb1939fabf..d997f8ebfa98 100644
--- a/drivers/mailbox/arm_mhuv2.c
+++ b/drivers/mailbox/arm_mhuv2.c
@@ -699,7 +699,9 @@ static irqreturn_t mhuv2_receiver_interrupt(int irq, void
*arg)
ret = IRQ_HANDLED;
}
- kfree(data);
+ if (!IS_ERR(data))
+ kfree(data);
+
return ret;
}
--
2.25.0.rc1.19.g042ed3e048af
Re: [PATCH] cpufreq: schedutil: Don't consider freq reduction to busy CPU if need_freq_update is set
On 19-02-21, 19:45, Yue Hu wrote:
> We will set next_f to next_freq(previous freq) if next_f is
> reduced for busy CPU. Then the next sugov_update_next_freq() will check
> if next_freq matches next_f if need_freq_update is not set.
> Obviously, we will do nothing for the case. And The related check to
> fast_switch_enabled and raw_spin_{lock,unlock} operations are
> unnecessary.
Right, but we will still need sugov_update_next_freq() to have the
same implementation regardless and so I am not sure if we should add
this change:
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 41e498b0008a..7289e1adab73 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -362,6 +362,9 @@ static void sugov_update_single_freq(struct
update_util_data *hook, u64 time,
* recently, as the reduction is likely to be premature then.
*/
if (sugov_cpu_is_busy(sg_cpu) && next_f < sg_policy->next_freq) {
+ if (!sg_policy->need_freq_update)
+ return;
+
next_f = sg_policy->next_freq;
/* Restore cached freq as next_freq has changed */
--
viresh
Re: [PATCH v8 0/3] CPUFreq: Add support for opp-sharing cpus
On 19-02-21, 19:16, Sudeep Holla wrote: > Hi Viresh, > > On Fri, Feb 19, 2021 at 09:49:44AM +0530, Viresh Kumar wrote: > > On 18-02-21, 22:23, Nicola Mazzucato wrote: > > > Hi Viresh, > > > > > > In this V8 I have addressed your comments: > > > - correct the goto in patch 1/3 > > > - improve comment in patch 2/3 for dev_pm_opp_get_opp_count() > > > > LGTM. I will apply them after the merge window is over. Thanks. > > I am planning to merge the series on scmi[1] which changes scmi-cpufreq.c > and will conflict with these changes I think. If possible either, > > 1. Share a branch with these changes that I can merge or > 2. I can take patch 1/3 and 2/3 with other scmi changes with your Ack. > > I am fine either way, let me know by v5.12-rc1 I have applied 3/3, you can take first two and add my Ack. Acked-by: Viresh Kumar -- viresh
Re: [PATCH V3 1/2] topology: Allow multiple entities to provide sched_freq_tick() callback
On 19-02-21, 09:44, Ionela Voinescu wrote: > On Friday 19 Feb 2021 at 10:28:23 (+0530), Viresh Kumar wrote: > > The very core routines (cpufreq_freq_transition_end() and > > cpufreq_driver_fast_switch()) of the cpufreq core call > > arch_set_freq_scale() today and this isn't going to change anytime > > soon. If something gets changed there someone will need to see other > > parts of the kernel which may get broken with that. > > > > Yes, but it won't really be straightforward to notice this breakage if > that happens, so in my opinion it was worth to keep that condition. Right, but chances of that happening are close to zero right now. I don't see any changes being made there in near future and so as we agreed, lets leave it as is. Btw, thanks for your feedback, it was indeed very valuable. -- viresh
Re: [PATCH] cpufreq: schedutil: Don't consider freq reduction to busy CPU if need_freq_update is set
On 19-02-21, 16:20, Yue Hu wrote: > However, we will skip the update if need_freq_update is not set. Not really, we will update freq periodically nevertheless, around every 10ms or something.. > And do the update if need_freq_update is set. Yeah, that breaks the periodic cycle to attend to some urgent request. > Note that there are unnecessary fast switch check and spin lock/unlock > operations in freq skip path. Maybe, I am not sure. We are all up for optimizations if there are any. > If we consider unnecessary behaviors above, then we should return right > away rather than continue to execute following code. As I said earlier, we may end up updating the frequency even if need_freq_update is unset. -- viresh
Re: [PATCH] cpufreq: schedutil: Don't consider freq reduction to busy CPU if need_freq_update is set
On 19-02-21, 14:41, Yue Hu wrote: > On Fri, 19 Feb 2021 09:39:33 +0530 > Viresh Kumar wrote: > > > On 19-02-21, 11:38, Yue Hu wrote: > > > There's a possibility: we will use the previous freq to update if > > > next_f is reduced for busy CPU if need_freq_update is set in > > > sugov_update_next_freq(). > > > > Right. > > > > > This possibility would happen now? And this > > > update is what we want if it happens? > > > > This is exactly what we want here, don't reduce speed for busy CPU, > > I understand it should not skip this update but set the same freq as > previous one again for the special case if need_freq_update is set. Am > i rt? The special check, about not reducing freq if CPU had been busy recently, doesn't have anything to do with need_freq_update. Though previously we added the need_freq_update check there to make sure we account for any recent policy min/max change and don't skip freq update anymore. That won't happen anymore and so we don't need any check here related to need_freq_update. If you still have doubt, please explain your concern in detail with an example as I am failing to understand it. -- viresh
Re: [PATCH V7 5/6] of: unittest: Create overlay_common.dtsi and testcases_common.dtsi
On 18-02-21, 23:20, Frank Rowand wrote: > Hi Viresh, > > I am in the wrong version with the comments below. You are at version 8 now. Yeah, it is fine. I have updated the patches already based on your comments. -- viresh
Re: [PATCH V3 1/2] topology: Allow multiple entities to provide sched_freq_tick() callback
On 18-02-21, 16:36, Ionela Voinescu wrote: > Yes, we don't care if there is no cpufreq driver, as the use of AMUs won't > get initialised either. But we do care if there is a cpufreq driver that > does not support frequency invariance, which is the example above. > > The intention with the patches that made cpufreq based invariance generic > a while back was for it to be present, seamlessly, for as many drivers as > possible, as a less than accurate invariance default method is still > better than nothing. Right. > So only a few drivers today don't support cpufreq based FI Only two AFAICT, both x86, and the AMU stuff doesn't conflict with them. drivers/cpufreq/intel_pstate.c drivers/cpufreq/longrun.c > but it's not a guarantee that it will stay this way. What do you mean by "no guarantee" here ? The very core routines (cpufreq_freq_transition_end() and cpufreq_driver_fast_switch()) of the cpufreq core call arch_set_freq_scale() today and this isn't going to change anytime soon. If something gets changed there someone will need to see other parts of the kernel which may get broken with that. I don't see any need of complicating other parts of the kernel like, amu or cppc code for that. They should be kept simple and they should assume cpufreq invariance will be supported as it is today. -- viresh
Re: [PATCH v8 0/3] CPUFreq: Add support for opp-sharing cpus
On 18-02-21, 22:23, Nicola Mazzucato wrote: > Hi Viresh, > > In this V8 I have addressed your comments: > - correct the goto in patch 1/3 > - improve comment in patch 2/3 for dev_pm_opp_get_opp_count() LGTM. I will apply them after the merge window is over. Thanks. -- viresh
Re: [PATCH] cpufreq: schedutil: Don't consider freq reduction to busy CPU if need_freq_update is set
On 19-02-21, 11:38, Yue Hu wrote: > There's a possibility: we will use the previous freq to update if next_f > is reduced for busy CPU if need_freq_update is set in > sugov_update_next_freq(). Right. > This possibility would happen now? And this > update is what we want if it happens? This is exactly what we want here, don't reduce speed for busy CPU, but we also need to make sure we are in the policy's valid range which cpufreq core will take care of. > This is related to another possible patch ready to send. I am not sure what's there to send now. -- viresh
Re: [PATCH] dt-bindings: cpufreq: cpufreq-qcom-hw: Document SM8350 CPUfreq compatible
On 18-02-21, 18:14, Vinod Koul wrote: > On 17-02-21, 10:19, Viresh Kumar wrote: > > On 16-02-21, 16:42, Vinod Koul wrote: > > > Add the CPUfreq compatible for SM8350 SoC along with note for using the > > > specific compatible for SoCs > > > > > > Signed-off-by: Vinod Koul > > > --- > > > Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git > > > a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt > > > b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt > > > index 9299028ee712..3eb3cee59d79 100644 > > > --- a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt > > > +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt > > > @@ -8,7 +8,9 @@ Properties: > > > - compatible > > > Usage: required > > > Value type: > > > - Definition: must be "qcom,cpufreq-hw" or "qcom,cpufreq-epss". > > > + Definition: must be "qcom,cpufreq-hw" or "qcom,cpufreq-epss" > > > + along with SoC specific compatible: > > > + "qcom,sm8350-cpufreq-epss", "qcom,cpufreq-epss" > > > > And why is SoC specific compatible required here ? Is the implementation on > > sm8350 any different than the ones using "qcom,cpufreq-epss" compatible ? > > > > FWIW, the same compatible string must be reused until the time there is > > difference in the hardware. The compatible string must be considered as a > > marker > > for a particular version of the hardware. > > Rob has indicated that we should use a SoC specific compatible and I > agree with that. We are using both soc and generic one here and driver > will be loaded for generic one. I am not sure of the context, lets see what Rob has to say on this. I believe we only need 1 compatible string here (whatever it is), as this is just one version of the hardware we are talking about. We already have 2 somehow and you are trying to add one more and I don't fell good about it. :( -- viresh
Re: [PATCH] cpufreq: exclude boost frequencies from valid count if not enabled
On 18-02-21, 10:03, Thara Gopinath wrote: > Scheduling a notifier for max frequency change from the qos framework should > do the work, right? Not that, but we need to increase/decrease cooling states at run time, create sysfs files/directories, etc. It isn't worth it. -- viresh
Re: [PATCH v7 1/3] scmi-cpufreq: Remove deferred probe
On 15-02-21, 07:51, Nicola Mazzucato wrote:
> The current implementation of the scmi_cpufreq_init() function returns
> -EPROBE_DEFER when the OPP table is not populated. In practice the
> cpufreq core cannot handle this error code.
> Therefore, fix the return value and clarify the error message.
>
> Reviewed-by: Ionela Voinescu
> Signed-off-by: Nicola Mazzucato
> ---
> drivers/cpufreq/scmi-cpufreq.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
> index 491a0a24fb1e..34bf2eb8d465 100644
> --- a/drivers/cpufreq/scmi-cpufreq.c
> +++ b/drivers/cpufreq/scmi-cpufreq.c
> @@ -155,9 +155,11 @@ static int scmi_cpufreq_init(struct cpufreq_policy
> *policy)
>
> nr_opp = dev_pm_opp_get_opp_count(cpu_dev);
> if (nr_opp <= 0) {
> - dev_dbg(cpu_dev, "OPP table is not ready, deferring probe\n");
> - ret = -EPROBE_DEFER;
> - goto out_free_opp;
Why change goto label as well ?
> + dev_err(cpu_dev, "%s: No OPPs for this device: %d\n",
> + __func__, ret);
> +
> + ret = -ENODEV;
> + goto out_free_priv;
> }
>
> priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> --
> 2.27.0
--
viresh
Re: [PATCH v7 2/3] scmi-cpufreq: Get opp_shared_cpus from opp-v2 for EM
On 15-02-21, 07:51, Nicola Mazzucato wrote:
> + /*
> + * Add OPPs only on those CPUs for which we haven't already done so.
> + */
> nr_opp = dev_pm_opp_get_opp_count(cpu_dev);
Please add a more detailed comment here explaining why you expect OPPs
to be present here in advance. i.e. you _may_ have policy per CPU even
though OPP core says OPPs are shared.. It is not straight forward to
catch otherwise.
> if (nr_opp <= 0) {
> - dev_err(cpu_dev, "%s: No OPPs for this device: %d\n",
> - __func__, ret);
> -
> - ret = -ENODEV;
> - goto out_free_priv;
> + ret = handle->perf_ops->device_opps_add(handle, cpu_dev);
> + if (ret) {
> + dev_warn(cpu_dev, "failed to add opps to the device\n");
> + goto out_free_cpumask;
> + }
> +
> + nr_opp = dev_pm_opp_get_opp_count(cpu_dev);
> + if (nr_opp <= 0) {
> + dev_err(cpu_dev, "%s: No OPPs for this device: %d\n",
> + __func__, ret);
> +
> + ret = -ENODEV;
> + goto out_free_opp;
> + }
> +
> + ret = dev_pm_opp_set_sharing_cpus(cpu_dev, opp_shared_cpus);
> + if (ret) {
> + dev_err(cpu_dev, "%s: failed to mark OPPs as shared:
> %d\n",
> + __func__, ret);
> +
> + goto out_free_opp;
> + }
> +
> + power_scale_mw = handle->perf_ops->power_scale_mw_get(handle);
> + em_dev_register_perf_domain(cpu_dev, nr_opp, _cb,
> + opp_shared_cpus, power_scale_mw);
> }
--
viresh
Re: [PATCH] cpufreq: schedutil: Don't consider freq reduction to busy CPU if need_freq_update is set
On 18-02-21, 16:25, Yue Hu wrote:
> From: Yue Hu
>
> For busy CPU case, we do not need to avoid freq reduction if limits
> change since commit 600f5badb78c ("cpufreq: schedutil: Don't skip
> freq update when limits change").
>
> Later, commit 23a881852f3e ("cpufreq: schedutil: Don't skip freq
> update if need_freq_update is set") discarded the need_freq_update
> check for special case of busy CPU because we won't abort a frequency
> update anymore if need_freq_update is set.
>
> That is nonlogical since we will not reduce the freq for busy CPU
> if the computed next_f is really reduced when limits change.
Schedutil governor will probably ask for a higher frequency than
allowed, but cpufreq core will clamp the request between policy
min/max before updating the frequency here.
We added the check in 600f5badb78c here earlier as there were chances
that we will abort the operation without reaching to cpufreq core,
which won't happen now.
--
viresh
Re: [PATCH V3 1/2] topology: Allow multiple entities to provide sched_freq_tick() callback
On 17-02-21, 00:24, Ionela Voinescu wrote:
> > diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> > index 1e47dfd465f8..47fca7376c93 100644
> > --- a/arch/arm64/kernel/topology.c
> > +++ b/arch/arm64/kernel/topology.c
> > @@ -240,7 +240,6 @@ static struct scale_freq_data amu_sfd = {
> >
> > static void amu_fie_setup(const struct cpumask *cpus)
> > {
> > - bool invariant;
> > int cpu;
> >
> > /* We are already set since the last insmod of cpufreq driver */
> > @@ -257,25 +256,10 @@ static void amu_fie_setup(const struct cpumask *cpus)
> >
> > cpumask_or(amu_fie_cpus, amu_fie_cpus, cpus);
> >
> > - invariant = topology_scale_freq_invariant();
> > -
> > - /* We aren't fully invariant yet */
> > - if (!invariant && !cpumask_equal(amu_fie_cpus, cpu_present_mask))
> > - return;
> > -
>
> You still need these checks, otherwise you could end up with only part
> of the CPUs setting a scale factor, when only part of the CPUs support
> AMUs and there is no cpufreq support for FIE.
Another look at it and here goes another reason (hope I don't have
another in-code comment somewhere else to kill this one) :)
We don't need to care for the reason you gave (which is a valid reason
otherwise), as we are talking specifically about amu_fie_setup() here
and it gets called from cpufreq policy-notifier. i.e. we won't support
AMUs without cpufreq being there in the first place and the same goes
for cppc-driver.
Does that sound reasonable ?
--
viresh
Re: [PATCH] cpufreq: exclude boost frequencies from valid count if not enabled
On 17-02-21, 10:32, Thara Gopinath wrote: > First of all, I am still unable to find this setting in the sysfs space. The driver needs to call cpufreq_enable_boost_support() for that. > Irrespective the ideal behavior here will be to change the cpufreq cooling > dev max state when this happens. Hmm.. recreating it every time boost frequency is enabled is like inviting trouble and it will be tricky. Maybe it can be done, I don't know.:) -- viresh
Re: [PATCH V3 1/2] topology: Allow multiple entities to provide sched_freq_tick() callback
On 17-02-21, 11:57, Ionela Voinescu wrote: > See a very useful comment someone added recently :) : > > """ > + /* > + * We don't need to handle CPUFREQ_REMOVE_POLICY event as the AMU > + * counters don't have any dependency on cpufreq driver once we have > + * initialized AMU support and enabled invariance. The AMU counters will > + * keep on working just fine in the absence of the cpufreq driver, and > + * for the CPUs for which there are no counters available, the last set > + * value of freq_scale will remain valid as that is the frequency those > + * CPUs are running at. > + */ > """ Lol... -- viresh
Re: [PATCH] opp: fix dev_pm_opp_set_rate for different frequency at the same opp level
On 16-02-21, 15:10, Jonathan Marek wrote:
> There is not "nothing to do" when the opp is the same. The frequency can
> be different from opp->rate.
>
> Fixes: 81c4d8a3c414 ("opp: Keep track of currently programmed OPP")
> Signed-off-by: Jonathan Marek
> ---
> drivers/opp/core.c | 7 +--
> drivers/opp/opp.h | 1 +
> 2 files changed, 6 insertions(+), 2 deletions(-)
Thanks, I have updated it a bit and applied, thanks.
--
viresh
-8<-
From: Jonathan Marek
Date: Tue, 16 Feb 2021 15:10:29 -0500
Subject: [PATCH] opp: Don't skip freq update for different frequency
We skip the OPP update if the current and target OPPs are same. This is
fine for the devices that don't support frequency but may cause issues
for the ones that need to program frequency.
An OPP entry doesn't really signify a single operating frequency but
rather the highest frequency at which the other properties of the OPP
entry apply. And we may reach here with different frequency values,
while all of them would point to the same OPP entry in the OPP table.
We just need to update the clock frequency in that case, though in order
to not add special exit points we reuse the code flow from a normal
path.
While at it, rearrange the conditionals in the 'if' statement to check
'enabled' flag at the end.
Fixes: 81c4d8a3c414 ("opp: Keep track of currently programmed OPP")
Signed-off-by: Jonathan Marek
[ Viresh: Improved commit log and subject, rename current_freq as
current_rate, document it, remove local variable and rearrange
code. ]
Signed-off-by: Viresh Kumar
---
drivers/opp/core.c | 8 +---
drivers/opp/opp.h | 2 ++
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index c3f3d9249cc5..c2689386a906 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -998,14 +998,15 @@ static int _set_opp(struct device *dev, struct opp_table
*opp_table,
old_opp = opp_table->current_opp;
/* Return early if nothing to do */
- if (opp_table->enabled && old_opp == opp) {
+ if (old_opp == opp && opp_table->current_rate == freq &&
+ opp_table->enabled) {
dev_dbg(dev, "%s: OPPs are same, nothing to do\n", __func__);
return 0;
}
dev_dbg(dev, "%s: switching OPP: Freq %lu -> %lu Hz, Level %u -> %u, Bw
%u -> %u\n",
- __func__, old_opp->rate, freq, old_opp->level, opp->level,
- old_opp->bandwidth ? old_opp->bandwidth[0].peak : 0,
+ __func__, opp_table->current_rate, freq, old_opp->level,
+ opp->level, old_opp->bandwidth ? old_opp->bandwidth[0].peak : 0,
opp->bandwidth ? opp->bandwidth[0].peak : 0);
scaling_down = _opp_compare_key(old_opp, opp);
@@ -1061,6 +1062,7 @@ static int _set_opp(struct device *dev, struct opp_table
*opp_table,
/* Make sure current_opp doesn't get freed */
dev_pm_opp_get(opp);
opp_table->current_opp = opp;
+ opp_table->current_rate = freq;
return ret;
}
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 9b9daf83b074..50fb9dced3c5 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -135,6 +135,7 @@ enum opp_table_access {
* @clock_latency_ns_max: Max clock latency in nanoseconds.
* @parsed_static_opps: Count of devices for which OPPs are initialized from
DT.
* @shared_opp: OPP is shared between multiple devices.
+ * @current_rate: Currently configured frequency.
* @current_opp: Currently configured OPP for the table.
* @suspend_opp: Pointer to OPP to be used during device suspend.
* @genpd_virt_dev_lock: Mutex protecting the genpd virtual device pointers.
@@ -184,6 +185,7 @@ struct opp_table {
unsigned int parsed_static_opps;
enum opp_table_access shared_opp;
+ unsigned long current_rate;
struct dev_pm_opp *current_opp;
struct dev_pm_opp *suspend_opp;
Re: [RESEND PATCH] MAINTAINERS: update thermal CPU cooling section
On 17-02-21, 11:59, Lukasz Luba wrote: > Update maintainers responsible for CPU cooling on Arm side. > > Signed-off-by: Lukasz Luba > --- > Hi Daniel, > > Please ignore the previous email and that this change with 'R'. > Javi will ack it later. > > Regards, > Lukasz > > MAINTAINERS | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index f32ebcff37d2..fe34f56acb0f 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -17774,7 +17774,7 @@ THERMAL/CPU_COOLING > M: Amit Daniel Kachhap > M: Daniel Lezcano > M: Viresh Kumar > -M: Javi Merino > +R: Lukasz Luba > L: linux...@vger.kernel.org > S: Supported > F: Documentation/driver-api/thermal/cpu-cooling-api.rst Good that we have one more reviewer for this :) Acked-by: Viresh Kumar -- viresh
Re: [PATCH V3 1/2] topology: Allow multiple entities to provide sched_freq_tick() callback
On 17-02-21, 11:30, Ionela Voinescu wrote: > The problem is not topology_scale_freq_invariant() but whether a scale > factor is set for some CPUs. > > Scenario (test system above): > - "AMUs" are only supported for [1-2], > - cpufreq_supports_freq_invariance() -> false > > What should happen: > - topology_scale_freq_invariant() -> false (passed) > - all CPUs should have their freq_scale unmodified (1024) - (failed) >because only 2 out of 6 CPUs have a method of setting a scale factor > > What does happen: > - arch_set_freq_tick() -> topology_set_freq_tick() will set a scale >factor for [1-2] based on AMUs. This should not happen. We will end >up with invariant signals for bigs and signals that are not freq >invariant for littles. Another case. cpufreq is included as a module and AMU is implemented partially. - first time cpufreq driver is inserted, we set up everything and freq_scale gets updated on ticks. - remove cpufreq driver, we are back in same situation. We can't control it that way.. Or we add another call layer in middle before the tick-handler gets called for AMU, which will check if we are fully invariant or not ? -- viresh
Re: [PATCH] thermal: cpufreq_cooling: freq_qos_update_request() returns < 0 on error
On 17-02-21, 10:29, Lukasz Luba wrote:
> On 2/17/21 5:48 AM, Viresh Kumar wrote:
> > freq_qos_update_request() returns 1 if the effective constraint value
> > has changed, 0 if the effective constraint value has not changed, or a
> > negative error code on failures.
> >
> > The frequency constraints for CPUs can be set by different parts of the
> > kernel. If the maximum frequency constraint set by other parts of the
> > kernel are set at a lower value than the one corresponding to cooling
> > state 0, then we will never be able to cool down the system as
> > freq_qos_update_request() will keep on returning 0 and we will skip
> > updating cpufreq_state and thermal pressure.
>
> To be precised, thermal pressure signal is not so important in this
> mechanism and the 'cpufreq_state' has changed recently:
Right, I wasn't concerned only about no thermal cooling, but both
thermal cooling and pressure.
> 236761f19a4f373354 thermal/drivers/cpufreq_cooling: Update cpufreq_state
> only if state has changed
This moved the assignment to a more logical place for me, i.e. not to
do that on errors, just that the block in which it landed may not get
called at all :(
> > Fix that by doing the updates even in the case where
> > freq_qos_update_request() returns 0, as we have effectively set the
> > constraint to a new value even if the consolidated value of the
> > actual constraint is unchanged because of external factors.
> >
> > Cc: v5.7+ # v5.7+
> > Reported-by: Thara Gopinath
> > Fixes: f12e4f66ab6a ("thermal/cpu-cooling: Update thermal pressure in case
> > of a maximum frequency capping")
>
> I'm not sure if that f12e4f is the root cause.
Hmm, depends on how we define the problem :)
If this was just about thermal-cooling not happening, then may be yes,
but to me it is rather about mishandled return value of
freq_qos_update_request() which has more than one side effects and so
I went for the main commit.
This is also important as f12e4f66ab6a got merged in 5.7 and 236761f19
merged in 5.11 and this patch needs to get applied in stable kernels
since 5.7 to fix it all.
> > Signed-off-by: Viresh Kumar
> > ---
> > Hi Guys,
> >
> > This needs to go in 5.12-rc.
> >
> > Thara, please give this a try and give your tested-by :).
> >
> > drivers/thermal/cpufreq_cooling.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
>
>
> Anyway, the fix LGTM. I will have to make sure that I'm CC'ed for these
> topic, so I can have a look (I missed somehow 236761f19)
>
> Reviewed-by: Lukasz Luba
> Tested-by: Lukasz Luba
Thanks.
--
viresh
Re: [PATCH] cpufreq: exclude boost frequencies from valid count if not enabled
Hi Thara, On 16-02-21, 19:00, Thara Gopinath wrote: > This is a fix for a regression observed on db845 platforms with 5.7-rc11 > kernel. On these platforms running stress tests with 5.11-rc7 kernel > causes big cpus to overheat and ultimately shutdown the system due to > hitting critical temperature (thermal throttling does not happen and > cur_state of cpufreq cooling device for big cpus remain stuck at 0 or max > frequency). > > This platform has boost opp defined for big cpus but boost mode itself is > disabled in the cpufreq driver. Hence the initial max frequency request > from cpufreq cooling device(cur_state) for big cpus is for boost > frequency(2803200) where as initial max frequency request from cpufreq > driver itself is for the highest non boost frequency (2649600). Okay. > qos > framework collates these two requests and puts the max frequency of big > cpus to 2649600 which the thermal framework is unaware of. It doesn't need to be aware of that. It sets its max frequency and other frameworks can put their own requests and the lowest one wins. In this case the other constraint came from cpufreq-core, which is fine. > Now during an > over heat event, with step-wise policy governor, thermal framework tries to > throttle the cpu and places a restriction on max frequency of the cpu to > cur_state - 1 Actually it is cur_state + 1 as the values are inversed here, cooling state 0 refers to highest frequency :) > which in this case 2649600. qos framework in turn tells the > cpufreq cooling device that max frequency of the cpu is already at 2649600 > and the cooling device driver returns doing nothing(cur_state of the > cooling device remains unchanged). And that's where the bug lies, I have sent proper fix for that now. > Thus thermal remains stuck in a loop and > never manages to actually throttle the cpu frequency. This ultimately leads > to system shutdown in case of a thermal overheat event on big cpus. > There are multiple possible fixes for this issue. Fundamentally,it is wrong > for cpufreq driver and cpufreq cooling device driver to show different > maximum possible state/frequency for a cpu. Not actually, cpufreq core changes the max supported frequency at runtime based on the availability of boost frequencies. cpufreq_table_count_valid_entries() is used at different places and it is implemented correctly. -- viresh
[PATCH] thermal: cpufreq_cooling: freq_qos_update_request() returns < 0 on error
freq_qos_update_request() returns 1 if the effective constraint value
has changed, 0 if the effective constraint value has not changed, or a
negative error code on failures.
The frequency constraints for CPUs can be set by different parts of the
kernel. If the maximum frequency constraint set by other parts of the
kernel are set at a lower value than the one corresponding to cooling
state 0, then we will never be able to cool down the system as
freq_qos_update_request() will keep on returning 0 and we will skip
updating cpufreq_state and thermal pressure.
Fix that by doing the updates even in the case where
freq_qos_update_request() returns 0, as we have effectively set the
constraint to a new value even if the consolidated value of the
actual constraint is unchanged because of external factors.
Cc: v5.7+ # v5.7+
Reported-by: Thara Gopinath
Fixes: f12e4f66ab6a ("thermal/cpu-cooling: Update thermal pressure in case of a
maximum frequency capping")
Signed-off-by: Viresh Kumar
---
Hi Guys,
This needs to go in 5.12-rc.
Thara, please give this a try and give your tested-by :).
drivers/thermal/cpufreq_cooling.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/thermal/cpufreq_cooling.c
b/drivers/thermal/cpufreq_cooling.c
index f5af2571f9b7..10af3341e5ea 100644
--- a/drivers/thermal/cpufreq_cooling.c
+++ b/drivers/thermal/cpufreq_cooling.c
@@ -485,7 +485,7 @@ static int cpufreq_set_cur_state(struct
thermal_cooling_device *cdev,
frequency = get_state_freq(cpufreq_cdev, state);
ret = freq_qos_update_request(_cdev->qos_req, frequency);
- if (ret > 0) {
+ if (ret >= 0) {
cpufreq_cdev->cpufreq_state = state;
cpus = cpufreq_cdev->policy->cpus;
max_capacity = arch_scale_cpu_capacity(cpumask_first(cpus));
--
2.25.0.rc1.19.g042ed3e048af
Re: [PATCH] opp: fix dev_pm_opp_set_rate for different frequency at the same opp level
On 16-02-21, 15:10, Jonathan Marek wrote:
> There is not "nothing to do" when the opp is the same. The frequency can
> be different from opp->rate.
I am sorry but I am not sure what are you trying to fix here and what exactly is
broken here. Can you provide a usecase for your platform where this doesn't work
like it used to ?
> Fixes: 81c4d8a3c414 ("opp: Keep track of currently programmed OPP")
> Signed-off-by: Jonathan Marek
> ---
> drivers/opp/core.c | 7 +--
> drivers/opp/opp.h | 1 +
> 2 files changed, 6 insertions(+), 2 deletions(-)
--
viresh
Re: [PATCH] dt-bindings: cpufreq: cpufreq-qcom-hw: Document SM8350 CPUfreq compatible
On 16-02-21, 16:42, Vinod Koul wrote: > Add the CPUfreq compatible for SM8350 SoC along with note for using the > specific compatible for SoCs > > Signed-off-by: Vinod Koul > --- > Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt > b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt > index 9299028ee712..3eb3cee59d79 100644 > --- a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt > +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt > @@ -8,7 +8,9 @@ Properties: > - compatible > Usage: required > Value type: > - Definition: must be "qcom,cpufreq-hw" or "qcom,cpufreq-epss". > + Definition: must be "qcom,cpufreq-hw" or "qcom,cpufreq-epss" > + along with SoC specific compatible: > + "qcom,sm8350-cpufreq-epss", "qcom,cpufreq-epss" And why is SoC specific compatible required here ? Is the implementation on sm8350 any different than the ones using "qcom,cpufreq-epss" compatible ? FWIW, the same compatible string must be reused until the time there is difference in the hardware. The compatible string must be considered as a marker for a particular version of the hardware. -- viresh
Re: [PATCH V3 1/2] topology: Allow multiple entities to provide sched_freq_tick() callback
On 17-02-21, 00:24, Ionela Voinescu wrote:
> I think it could be merged in patch 1/2 as it's part of enabling the use
> of multiple sources of information for FIE. Up to you!
Sure.
> > static void amu_fie_setup(const struct cpumask *cpus)
> > {
> > - bool invariant;
> > int cpu;
> >
> > /* We are already set since the last insmod of cpufreq driver */
> > @@ -257,25 +256,10 @@ static void amu_fie_setup(const struct cpumask *cpus)
> >
> > cpumask_or(amu_fie_cpus, amu_fie_cpus, cpus);
> >
> > - invariant = topology_scale_freq_invariant();
> > -
> > - /* We aren't fully invariant yet */
> > - if (!invariant && !cpumask_equal(amu_fie_cpus, cpu_present_mask))
> > - return;
> > -
>
> You still need these checks, otherwise you could end up with only part
> of the CPUs setting a scale factor, when only part of the CPUs support
> AMUs and there is no cpufreq support for FIE.
Both supports_scale_freq_counters() and topology_scale_freq_invariant() take
care of this now and they will keep reporting the system as invariant until the
time all the CPUs have counters (in absence of cpufreq).
The topology_set_scale_freq_source() API is supposed to be called multiple
times, probably once for each policy and so I don't see a need of these checks
anymore.
> Small(ish) optimisation at the beginning of this function:
>
> if (cpumask_empty(_freq_counters_mask))
> scale_freq_invariant = topology_scale_freq_invariant();
>
> This will save you a call to rebuild_sched_domains_energy(), which is
> quite expensive, when cpufreq supports FIE and we also have counters.
Good Point.
> After comments addressed,
>
> Reviewed-by: Ionela Voinescu
Thanks.
> Tested-by: Ionela Voinescu
Just out of curiosity, what exactly did you test and what was the setup ? :)
--
viresh
Re: [PATCH V7 6/6] of: unittest: Statically apply overlays using fdtoverlay
On 09-02-21, 15:40, Viresh Kumar wrote: > And after decent amount of effort understanding how to do this, I > finally did it in a not so efficient way, I am sure you can help > improving it :) Ping! Also, where do we send patches for dt-schema ? Which list ? > Author: Viresh Kumar > Date: Tue Feb 9 12:19:50 2021 +0530 > > dt-validate: Skip "required property" checks for overlays > > The overlays may not carry the required properties and would depend on > the base dtb to carry those, there is no point raising those errors > here. > > Signed-off-by: Viresh Kumar > --- > tools/dt-validate | 17 + > 1 file changed, 17 insertions(+) > > diff --git a/tools/dt-validate b/tools/dt-validate > index 410b0538ef47..c6117504f1d1 100755 > --- a/tools/dt-validate > +++ b/tools/dt-validate > @@ -80,6 +80,23 @@ show_unmatched = False >(filename, line, col, fullname, > node['compatible']), file=sys.stderr) > continue > > +if nodename == '/': > +is_fragment = False > +for name in node.items(): > +if name[0] == 'fragment@0': > +is_fragment = True > +break; > + > +if is_fragment == True: > +if 'required property' in error.message: > +continue > +elif error.context: > +for e in error.context: > +if not 'required property' in > e.message: > +break > +else: > +continue > + > print(dtschema.format_error(filename, error, > nodename=nodename, verbose=verbose) + > '\n\tFrom schema: ' + schema['$filename'], > file=sys.stderr) -- viresh
Re: [PATCH] cpufreq: schedutil: Don't use the limits_changed flag any more
On 14-02-21, 11:44, Yue Hu wrote:
> On Fri, 12 Feb 2021 17:14:03 +0100
> "Rafael J. Wysocki" wrote:
> > This may be running in parallel with sugov_update_next_freq() on a
> > different CPU, so the latter may clear need_freq_update right after it
> > has been set here unless I'm overlooking something.
>
> Whether this logic is also happening for limits_changed in
> sugo_should_update_freq() or not?
It is but it shouldn't have any side effects as we calculate the next
frequency after cleaning the limits_changed flag. Your patch would
have been fine, but it is not anymore because of commit 23a881852f3e
("cpufreq: schedutil: Don't skip freq update if need_freq_update is
set").
It made a considerable change after which your patch adds a bug. With
23a881852f3e, need_freq_update is updated/cleared after the next
frequency is calculated, while earlier it was cleared before it. And
so even with the race condition taking place, there were no issues.
--
viresh
[GIT PULL] Remove oprofile and dcookies for v5.12
Hi Linus, The following changes since commit 7c53f6b671f4aba70ff15e1b05148b10d58c2837: Linux 5.11-rc3 (2021-01-10 14:34:50 -0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/linux.git tags/oprofile-removal-5.12 for you to fetch changes up to be65de6b03aa638c46ea51e9d11a92e4914d8103: fs: Remove dcookies support (2021-01-29 10:06:46 +0530) Remove oprofile and dcookies support The "oprofile" user-space tools don't use the kernel OPROFILE support any more, and haven't in a long time. User-space has been converted to the perf interfaces. The dcookies stuff is only used by the oprofile code. Now that oprofile's support is getting removed from the kernel, there is no need for dcookies as well. Remove kernel's old oprofile and dcookies support. -------- Viresh Kumar (18): arch: alpha: Remove CONFIG_OPROFILE support arch: arm: Remove CONFIG_OPROFILE support arch: arc: Remove CONFIG_OPROFILE support arch: hexagon: Don't select HAVE_OPROFILE arch: ia64: Remove CONFIG_OPROFILE support arch: ia64: Remove rest of perfmon support arch: microblaze: Remove CONFIG_OPROFILE support arch: mips: Remove CONFIG_OPROFILE support arch: parisc: Remove CONFIG_OPROFILE support arch: powerpc: Stop building and using oprofile arch: powerpc: Remove oprofile arch: s390: Remove CONFIG_OPROFILE support arch: sh: Remove CONFIG_OPROFILE support arch: sparc: Remove CONFIG_OPROFILE support arch: x86: Remove CONFIG_OPROFILE support arch: xtensa: Remove CONFIG_OPROFILE support drivers: Remove CONFIG_OPROFILE support fs: Remove dcookies support Documentation/RCU/NMI-RCU.rst |3 +- Documentation/admin-guide/kernel-parameters.txt| 14 - Documentation/kbuild/makefiles.rst |1 - Documentation/process/magic-number.rst |1 - .../translations/it_IT/process/magic-number.rst|1 - .../translations/zh_CN/process/magic-number.rst|1 - MAINTAINERS| 11 - arch/Kconfig | 32 - arch/alpha/Kconfig |1 - arch/alpha/Makefile|1 - arch/alpha/oprofile/Makefile | 20 - arch/alpha/oprofile/common.c | 189 --- arch/alpha/oprofile/op_impl.h | 55 - arch/alpha/oprofile/op_model_ev4.c | 114 -- arch/alpha/oprofile/op_model_ev5.c | 209 --- arch/alpha/oprofile/op_model_ev6.c | 101 -- arch/alpha/oprofile/op_model_ev67.c| 261 --- arch/arc/Kconfig |1 - arch/arc/Makefile |2 - arch/arc/oprofile/Makefile | 10 - arch/arc/oprofile/common.c | 23 - arch/arm/Kconfig |1 - arch/arm/Makefile |2 - arch/arm/configs/bcm2835_defconfig |1 - arch/arm/configs/cns3420vb_defconfig |1 - arch/arm/configs/corgi_defconfig |1 - arch/arm/configs/imx_v4_v5_defconfig |1 - arch/arm/configs/keystone_defconfig|1 - arch/arm/configs/multi_v5_defconfig|1 - arch/arm/configs/mv78xx0_defconfig |1 - arch/arm/configs/mvebu_v5_defconfig|1 - arch/arm/configs/omap1_defconfig |1 - arch/arm/configs/omap2plus_defconfig |1 - arch/arm/configs/orion5x_defconfig |1 - arch/arm/configs/pxa_defconfig |1 - arch/arm/configs/qcom_defconfig|1 - arch/arm/configs/socfpga_defconfig |1 - arch/arm/configs/spitz_defconfig |1 - arch/arm/configs/vexpress_defconfig|1 - arch/arm/oprofile/Makefile | 14 - arch/arm/oprofile/common.c | 132 -- arch/hexagon/Kconfig |1 - arch/ia64/Kconfig |1 - arch/ia64/Makefile |1 - arch/ia64/configs/bigsur_defconfig |1 - arch/ia64/include/asm/hw_irq.h |1 - arch/ia64/include/asm/perfmon.h| 111 -- arch/ia64/include/uapi/asm/perfmon.h | 178 -- arch/ia64/include/uapi/asm/perfmon_default_smpl.h | 84 - arch/ia64/kernel/palinfo.c | 41 - arch/ia64/kernel/perfmon_default_smpl.c| 297 arch/ia64/kernel/perfmon
[PATCH V8 3/4] of: unittest: Create overlay_common.dtsi and testcases_common.dtsi
In order to build-test the same unit-test files using fdtoverlay tool,
move the device nodes from the existing overlay_base.dts and
testcases_common.dts files to .dtsi counterparts. The .dts files now
include the new .dtsi files, resulting in exactly the same behavior as
earlier.
The .dtsi files can now be reused for compile time tests using
fdtoverlay (will be done by a later commit).
This is required because the base files passed to fdtoverlay tool
shouldn't be overlays themselves (i.e. shouldn't have the /plugin/;
tag).
Note that this commit also moves "testcase-device2" node to
testcases.dts from tests-interrupts.dtsi, as this node has a deliberate
error in it and is only relevant for runtime testing done with
unittest.c.
Signed-off-by: Viresh Kumar
---
drivers/of/unittest-data/overlay_base.dts | 90 +-
drivers/of/unittest-data/overlay_common.dtsi | 91 +++
drivers/of/unittest-data/testcases.dts| 18 ++--
.../of/unittest-data/testcases_common.dtsi| 19
.../of/unittest-data/tests-interrupts.dtsi| 7 --
5 files changed, 118 insertions(+), 107 deletions(-)
create mode 100644 drivers/of/unittest-data/overlay_common.dtsi
create mode 100644 drivers/of/unittest-data/testcases_common.dtsi
diff --git a/drivers/of/unittest-data/overlay_base.dts
b/drivers/of/unittest-data/overlay_base.dts
index 99ab9d12d00b..ab9014589c5d 100644
--- a/drivers/of/unittest-data/overlay_base.dts
+++ b/drivers/of/unittest-data/overlay_base.dts
@@ -2,92 +2,4 @@
/dts-v1/;
/plugin/;
-/*
- * Base device tree that overlays will be applied against.
- *
- * Do not add any properties in node "/".
- * Do not add any nodes other than "/testcase-data-2" in node "/".
- * Do not add anything that would result in dtc creating node "/__fixups__".
- * dtc will create nodes "/__symbols__" and "/__local_fixups__".
- */
-
-/ {
- testcase-data-2 {
- #address-cells = <1>;
- #size-cells = <1>;
-
- electric_1: substation@100 {
- compatible = "ot,big-volts-control";
- reg = < 0x0100 0x100 >;
- status = "disabled";
-
- hvac_1: hvac-medium-1 {
- compatible = "ot,hvac-medium";
- heat-range = < 50 75 >;
- cool-range = < 60 80 >;
- };
-
- spin_ctrl_1: motor-1 {
- compatible = "ot,ferris-wheel-motor";
- spin = "clockwise";
- rpm_avail = < 50 >;
- };
-
- spin_ctrl_2: motor-8 {
- compatible = "ot,roller-coaster-motor";
- };
- };
-
- rides_1: fairway-1 {
- #address-cells = <1>;
- #size-cells = <1>;
- compatible = "ot,rides";
- status = "disabled";
- orientation = < 127 >;
-
- ride@100 {
- #address-cells = <1>;
- #size-cells = <1>;
- compatible = "ot,roller-coaster";
- reg = < 0x0100 0x100 >;
- hvac-provider = < _1 >;
- hvac-thermostat = < 29 > ;
- hvac-zones = < 14 >;
- hvac-zone-names = "operator";
- spin-controller = < _ctrl_2 5 _ctrl_2
7 >;
- spin-controller-names = "track_1", "track_2";
- queues = < 2 >;
-
- track@30 {
- reg = < 0x0030 0x10 >;
- };
-
- track@40 {
- reg = < 0x0040 0x10 >;
- };
-
- };
- };
-
- lights_1: lights@3 {
- compatible = "ot,work-lights";
- reg = < 0x0003 0x1000 >;
- status = "disabled";
- };
-
- lights_2: lights@4 {
- compatible = "ot,show-lights";
- reg = < 0x0004 0x1000 >;
- status = "disabled";
- rate = < 13 138 >;
-
[PATCH V8 4/4] of: unittest: Statically apply overlays using fdtoverlay
Now that fdtoverlay is part of the kernel build, start using it to test the unitest overlays we have by applying them statically. Create two new base files static_base_1.dts and static_base_2.dts which includes other .dtsi files. Some unittest overlays deliberately contain errors that unittest checks for. These overlays will cause fdtoverlay to fail, and are thus not included for static builds. Signed-off-by: Viresh Kumar --- drivers/of/unittest-data/Makefile | 50 ++ drivers/of/unittest-data/static_base_1.dts | 4 ++ drivers/of/unittest-data/static_base_2.dts | 4 ++ 3 files changed, 58 insertions(+) create mode 100644 drivers/of/unittest-data/static_base_1.dts create mode 100644 drivers/of/unittest-data/static_base_2.dts diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile index 009f4045c8e4..f88b2f48f172 100644 --- a/drivers/of/unittest-data/Makefile +++ b/drivers/of/unittest-data/Makefile @@ -34,7 +34,57 @@ DTC_FLAGS_overlay += -@ DTC_FLAGS_overlay_bad_phandle += -@ DTC_FLAGS_overlay_bad_symbol += -@ DTC_FLAGS_overlay_base += -@ +DTC_FLAGS_static_base_1 += -@ +DTC_FLAGS_static_base_2 += -@ DTC_FLAGS_testcases += -@ # suppress warnings about intentional errors DTC_FLAGS_testcases += -Wno-interrupts_property + +# Apply overlays statically with fdtoverlay. This is a build time test that +# the overlays can be applied successfully by fdtoverlay. This does not +# guarantee that the overlays can be applied successfully at run time by +# unittest, but it provides a bit of build time test coverage for those +# who do not execute unittest. +# +# The overlays are applied on top of static_base_1.dtb and static_base_2.dtb to +# create static_test_1.dtb and static_test_2.dtb. If fdtoverlay detects an +# error than the kernel build will fail. static_test_1.dtb and +# static_test_2.dtb are not consumed by unittest. +# +# Some unittest overlays deliberately contain errors that unittest checks for. +# These overlays will cause fdtoverlay to fail, and are thus not included +# in the static test: +#overlay_bad_add_dup_node.dtbo \ +#overlay_bad_add_dup_prop.dtbo \ +#overlay_bad_phandle.dtbo \ +#overlay_bad_symbol.dtbo \ + +apply_static_overlay_1 := overlay_0.dtbo \ + overlay_1.dtbo \ + overlay_2.dtbo \ + overlay_3.dtbo \ + overlay_4.dtbo \ + overlay_5.dtbo \ + overlay_6.dtbo \ + overlay_7.dtbo \ + overlay_8.dtbo \ + overlay_9.dtbo \ + overlay_10.dtbo \ + overlay_11.dtbo \ + overlay_12.dtbo \ + overlay_13.dtbo \ + overlay_15.dtbo \ + overlay_gpio_01.dtbo \ + overlay_gpio_02a.dtbo \ + overlay_gpio_02b.dtbo \ + overlay_gpio_03.dtbo \ + overlay_gpio_04a.dtbo \ + overlay_gpio_04b.dtbo + +apply_static_overlay_2 := overlay.dtbo + +static_test_1-dtbs := static_base_1.dtb $(apply_static_overlay_1) +static_test_2-dtbs := static_base_2.dtb $(apply_static_overlay_2) + +dtb-$(CONFIG_OF_OVERLAY) += static_test_1.dtb static_test_2.dtb diff --git a/drivers/of/unittest-data/static_base_1.dts b/drivers/of/unittest-data/static_base_1.dts new file mode 100644 index ..10556cb3f01f --- /dev/null +++ b/drivers/of/unittest-data/static_base_1.dts @@ -0,0 +1,4 @@ +// SPDX-License-Identifier: GPL-2.0 +/dts-v1/; + +#include "testcases_common.dtsi" diff --git a/drivers/of/unittest-data/static_base_2.dts b/drivers/of/unittest-data/static_base_2.dts new file mode 100644 index ..b0ea9504d6f3 --- /dev/null +++ b/drivers/of/unittest-data/static_base_2.dts @@ -0,0 +1,4 @@ +// SPDX-License-Identifier: GPL-2.0 +/dts-v1/; + +#include "overlay_common.dtsi" -- 2.25.0.rc1.19.g042ed3e048af
[PATCH V8 2/4] kbuild: Add generic rule to apply fdtoverlay
From: Rob Herring Add a generic rule to apply fdtoverlay in Makefile.lib, so every platform doesn't need to carry the complex rule. The platform's Makefile only needs to have this now: DTC_FLAGS_foo_base += -@ foo-dtbs := foo_base.dtb foo_overlay1.dtbo foo_overlay2.dtbo dtb-y := foo.dtb We don't want to run schema checks on foo.dtb (as foo.dts doesn't exist) and the Makefile is updated accordingly. Signed-off-by: Rob Herring Co-developed-by: Viresh Kumar Signed-off-by: Viresh Kumar --- scripts/Makefile.lib | 24 ++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index fa0db696120f..3c450bfec015 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -66,6 +66,10 @@ multi-used := $(multi-used-y) $(multi-used-m) real-obj-y := $(foreach m, $(obj-y), $(if $(strip $($(m:.o=-objs)) $($(m:.o=-y)) $($(m:.o=-))),$($(m:.o=-objs)) $($(m:.o=-y)),$(m))) real-obj-m := $(foreach m, $(obj-m), $(if $(strip $($(m:.o=-objs)) $($(m:.o=-y)) $($(m:.o=-m)) $($(m:.o=-))),$($(m:.o=-objs)) $($(m:.o=-y)) $($(m:.o=-m)),$(m))) +# List all dtbs to be generated by fdtoverlay +overlay-y := $(foreach m,$(dtb-y), $(if $(strip $($(m:.dtb=-dtbs))),$(m),)) +overlay-$(CONFIG_OF_ALL_DTBS) += $(foreach m,$(dtb-), $(if $(strip $($(m:.dtb=-dtbs))),$(m),)) + always-y += $(always-m) # hostprogs-always-y += foo @@ -80,14 +84,21 @@ userprogs += $(userprogs-always-y) $(userprogs-always-m) always-y += $(userprogs-always-y) $(userprogs-always-m) # DTB +# Add base dtb and overlay dtbo +dtb-y += $(foreach m,$(overlay-y), $($(m:.dtb=-dtbs))) + # If CONFIG_OF_ALL_DTBS is enabled, all DT blobs are built dtb-$(CONFIG_OF_ALL_DTBS) += $(dtb-) extra-y+= $(dtb-y) ifneq ($(CHECK_DTBS),) -extra-y += $(patsubst %.dtb,%.dt.yaml, $(dtb-y)) -extra-y += $(patsubst %.dtbo,%.dt.yaml, $(dtb-y)) +# Don't run schema checks for dtbs created by fdtoverlay as they don't +# have corresponding dts files. +dt-yaml-y := $(filter-out $(overlay-y),$(dtb-y)) + +extra-y += $(patsubst %.dtb,%.dt.yaml, $(dt-yaml-y)) +extra-y += $(patsubst %.dtbo,%.dt.yaml, $(dt-yaml-y)) endif # Add subdir path @@ -331,6 +342,15 @@ $(obj)/%.dtb: $(src)/%.dts $(DTC) FORCE $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE $(call if_changed_dep,dtc) +overlay-y := $(addprefix $(obj)/, $(overlay-y)) + +quiet_cmd_fdtoverlay = DTOVL $@ + cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $(real-prereqs) + +$(overlay-y): FORCE + $(call if_changed,fdtoverlay) +$(call multi_depend, $(overlay-y), .dtb, -dtbs) + DT_CHECKER ?= dt-validate DT_BINDING_DIR := Documentation/devicetree/bindings # DT_TMP_SCHEMA may be overridden from Documentation/devicetree/bindings/Makefile -- 2.25.0.rc1.19.g042ed3e048af
[PATCH V8 0/4] dt: Add fdtoverlay rule and statically build unittest
Hi, This patchset adds a generic rule for applying overlays using fdtoverlay tool and then updates unittests to get built statically using the same. V7->V8: - Patch 1 is new. - Platforms need to use dtb-y += foo.dtb instead of overlay-y += foo.dtb. - Use multi_depend instead of .SECONDEXPANSION. - Use dtb-y for unittest instead of overlay-y. - Rename the commented dtb filess in unittest Makefile as .dtbo. - Improved Makefile code (I am learning a lot every day :) V6->V7: - Dropped the first 4 patches, already merged. - Patch 1/3 is new, suggested by Rob and slightly modified by me. - Adapt Patch 3/3 to the new rule and name the overlay dtbs as .dtbo. -- Viresh Rob Herring (1): kbuild: Add generic rule to apply fdtoverlay Viresh Kumar (3): kbuild: Simplify builds with CONFIG_OF_ALL_DTBS of: unittest: Create overlay_common.dtsi and testcases_common.dtsi of: unittest: Statically apply overlays using fdtoverlay drivers/of/unittest-data/Makefile | 50 ++ drivers/of/unittest-data/overlay_base.dts | 90 +- drivers/of/unittest-data/overlay_common.dtsi | 91 +++ drivers/of/unittest-data/static_base_1.dts| 4 + drivers/of/unittest-data/static_base_2.dts| 4 + drivers/of/unittest-data/testcases.dts| 18 ++-- .../of/unittest-data/testcases_common.dtsi| 19 .../of/unittest-data/tests-interrupts.dtsi| 7 -- scripts/Makefile.lib | 29 +- 9 files changed, 200 insertions(+), 112 deletions(-) create mode 100644 drivers/of/unittest-data/overlay_common.dtsi create mode 100644 drivers/of/unittest-data/static_base_1.dts create mode 100644 drivers/of/unittest-data/static_base_2.dts create mode 100644 drivers/of/unittest-data/testcases_common.dtsi -- 2.25.0.rc1.19.g042ed3e048af
[PATCH V8 1/4] kbuild: Simplify builds with CONFIG_OF_ALL_DTBS
We update 'extra-y' based on CONFIG_OF_ALL_DTBS three times. It would be far more straight forward if we rather update dtb-y to include all .dtb files if CONFIG_OF_ALL_DTBS is enabled. Signed-off-by: Viresh Kumar --- scripts/Makefile.lib | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index b00855b247e0..fa0db696120f 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -81,14 +81,13 @@ always-y += $(userprogs-always-y) $(userprogs-always-m) # DTB # If CONFIG_OF_ALL_DTBS is enabled, all DT blobs are built +dtb-$(CONFIG_OF_ALL_DTBS) += $(dtb-) + extra-y+= $(dtb-y) -extra-$(CONFIG_OF_ALL_DTBS)+= $(dtb-) ifneq ($(CHECK_DTBS),) extra-y += $(patsubst %.dtb,%.dt.yaml, $(dtb-y)) extra-y += $(patsubst %.dtbo,%.dt.yaml, $(dtb-y)) -extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtb,%.dt.yaml, $(dtb-)) -extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtbo,%.dt.yaml, $(dtb-)) endif # Add subdir path -- 2.25.0.rc1.19.g042ed3e048af
Re: [PATCH 1/2] staging: greybus: Fixed alignment issue in hid.c
On 12-02-21, 10:52, Johan Hovold wrote: > But what will the checkpatch crew then work on? Lol.. > Better staging than the rest of the kernel. [ /me wondering on who stops them from sending patches for rest of the kernel ? ] -- viresh
Re: [PATCH 1/2] staging: greybus: Fixed alignment issue in hid.c
On 12-02-21, 10:17, Greg KH wrote:
> On Fri, Feb 12, 2021 at 02:39:26PM +0530, Viresh Kumar wrote:
> > On 12-02-21, 13:48, Pritthijit Nath wrote:
> > > This change fixes a checkpatch CHECK style issue for "Alignment should
> > > match
> > > open parenthesis".
> > >
> > > Signed-off-by: Pritthijit Nath
> > > ---
> > > drivers/staging/greybus/hid.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/staging/greybus/hid.c b/drivers/staging/greybus/hid.c
> > > index ed706f39e87a..a56c3fb5d35a 100644
> > > --- a/drivers/staging/greybus/hid.c
> > > +++ b/drivers/staging/greybus/hid.c
> > > @@ -221,8 +221,8 @@ static void gb_hid_init_reports(struct gb_hid *ghid)
> > > }
> > >
> > > static int __gb_hid_get_raw_report(struct hid_device *hid,
> > > - unsigned char report_number, __u8 *buf, size_t count,
> > > - unsigned char report_type)
> > > +unsigned char report_number, __u8 *buf,
> > > size_t count,
> > > +unsigned char report_type)
> > > {
> > > struct gb_hid *ghid = hid->driver_data;
> > > int ret;
> >
> > I can't even count the number of attempts we have seen in previous
> > years to make checkpatch --strict happy for greybus.
> >
> > I say we make this change once and for all across greybus, so we don't
> > have to review or NAK someone afterwards.
> >
> > Should I send a patch for this ?
>
> Sure, but note that over time, checkpatch adds new things so there will
> always be something to change in here, until we move it out of the
> drivers/staging/ area :)
Right, though I wasn't worried about other checkpatch warning but
specially the "alignment - parenthesis" one. Everyone (specially
newbies) want to fix that everywhere :)
--
viresh
Re: [PATCH 1/2] staging: greybus: Fixed alignment issue in hid.c
On 12-02-21, 13:48, Pritthijit Nath wrote:
> This change fixes a checkpatch CHECK style issue for "Alignment should match
> open parenthesis".
>
> Signed-off-by: Pritthijit Nath
> ---
> drivers/staging/greybus/hid.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/greybus/hid.c b/drivers/staging/greybus/hid.c
> index ed706f39e87a..a56c3fb5d35a 100644
> --- a/drivers/staging/greybus/hid.c
> +++ b/drivers/staging/greybus/hid.c
> @@ -221,8 +221,8 @@ static void gb_hid_init_reports(struct gb_hid *ghid)
> }
>
> static int __gb_hid_get_raw_report(struct hid_device *hid,
> - unsigned char report_number, __u8 *buf, size_t count,
> - unsigned char report_type)
> +unsigned char report_number, __u8 *buf,
> size_t count,
> +unsigned char report_type)
> {
> struct gb_hid *ghid = hid->driver_data;
> int ret;
I can't even count the number of attempts we have seen in previous
years to make checkpatch --strict happy for greybus.
I say we make this change once and for all across greybus, so we don't
have to review or NAK someone afterwards.
Should I send a patch for this ?
--
viresh
Re: [PATCH 2/2] staging: greybus: Fixed a misspelling in hid.c
On 12-02-21, 09:54, Greg KH wrote: > On Fri, Feb 12, 2021 at 09:44:04AM +0100, Johan Hovold wrote: > > On Fri, Feb 12, 2021 at 01:48:35PM +0530, Pritthijit Nath wrote: > > > Fixed the spelling of 'transfered' to 'transferred'. > > > > > > Signed-off-by: Pritthijit Nath > > > --- > > > drivers/staging/greybus/hid.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/staging/greybus/hid.c b/drivers/staging/greybus/hid.c > > > index a56c3fb5d35a..6b19ff4743a9 100644 > > > --- a/drivers/staging/greybus/hid.c > > > +++ b/drivers/staging/greybus/hid.c > > > @@ -254,7 +254,7 @@ static int __gb_hid_output_raw_report(struct > > > hid_device *hid, __u8 *buf, > > > > > > ret = gb_hid_set_report(ghid, report_type, report_id, buf, len); > > > if (report_id && ret >= 0) > > > - ret++; /* add report_id to the number of transfered bytes */ > > > + ret++; /* add report_id to the number of transferrid bytes */ > > > > You now misspelled transferred in a different way. > > Oops, will go revert this, I need more coffee... Yeah, its Friday.. You need a break too :) -- viresh
Re: [PATCH V7 1/3] kbuild: Add generic rule to apply fdtoverlay
On 12-02-21, 12:07, Masahiro Yamada wrote: > BTW, I do not know how to use overlay. > Do we apply overlay in the build time? Ideally it can be applied at both build time and runtime, but we haven't allowed the runtime way until now in kernel. This patchset is all about applying it at build time. > If so, I do not know what the benefit of overlay is. > Or is this just for build testing? For now the main benefit of using them is that we can keep stuff in separate files without including each other. For example a primary board may or may not have an extension board connected to it. Without overlays we will have this many dtbs for this simple case: 1. primary.dtb 2. extension.dtb 3. primary-includes-extension.dtb With overlays we will have the first two. Now the same extension can be applied to lots of boards and multiple extensions can be applied to the same primary board. This just complicates the process of managing dtbs. > I just thought this was done in the boot time, > for example, in U-Boot or something. Yes, bootloader can do it as well. -- viresh
Re: [PATCH v1 2/9] cpufreq: sfi-cpufreq: Remove driver for deprecated firmware
On 11-02-21, 15:40, Andy Shevchenko wrote: > SFI-based platforms are gone. So does this driver. > > Signed-off-by: Andy Shevchenko > Acked-by: Linus Walleij > --- > drivers/cpufreq/Kconfig.x86 | 10 --- > drivers/cpufreq/Makefile | 1 - > drivers/cpufreq/sfi-cpufreq.c | 127 -- > 3 files changed, 138 deletions(-) > delete mode 100644 drivers/cpufreq/sfi-cpufreq.c Acked-by: Viresh Kumar -- viresh
Re: [greybus-dev] [PATCH 1/1] staging: greybus: Added do - while in multi statement macro
On 11-02-21, 11:00, Greg KH wrote: > On Thu, Feb 11, 2021 at 03:24:44PM +0530, Hemansh Agnihotri wrote: > > This patch add fixes an checkpatch error for "Macros with multiple > > statements > > should be enclosed in a do - while loop" > > > > Signed-off-by: Hemansh Agnihotri > > Any reason you didn't test-build your patch before sending it out? > > That's a bit rude to reviewers :( I also wonder how two people stumbled upon the exact same thing at the same time. Copy/paste ? https://lore.kernel.org/lkml/20210210221439.3489-2-yildirim.fa...@gmail.com/ And of course NAK for the patch. The macro is used outside of any other routine, and is actually used to create routines. No do-while required here. -- viresh
[PATCH V7 3/3] of: unittest: Statically apply overlays using fdtoverlay
Now that fdtoverlay is part of the kernel build, start using it to test the unitest overlays we have by applying them statically. Create two new base files static_base_1.dts and static_base_2.dts which includes other .dtsi files. Some unittest overlays deliberately contain errors that unittest checks for. These overlays will cause fdtoverlay to fail, and are thus not included for static builds. Signed-off-by: Viresh Kumar --- drivers/of/unittest-data/Makefile | 50 ++ drivers/of/unittest-data/static_base_1.dts | 4 ++ drivers/of/unittest-data/static_base_2.dts | 4 ++ 3 files changed, 58 insertions(+) create mode 100644 drivers/of/unittest-data/static_base_1.dts create mode 100644 drivers/of/unittest-data/static_base_2.dts diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile index 009f4045c8e4..1d6029e722c0 100644 --- a/drivers/of/unittest-data/Makefile +++ b/drivers/of/unittest-data/Makefile @@ -34,7 +34,57 @@ DTC_FLAGS_overlay += -@ DTC_FLAGS_overlay_bad_phandle += -@ DTC_FLAGS_overlay_bad_symbol += -@ DTC_FLAGS_overlay_base += -@ +DTC_FLAGS_static_base_1 += -@ +DTC_FLAGS_static_base_2 += -@ DTC_FLAGS_testcases += -@ # suppress warnings about intentional errors DTC_FLAGS_testcases += -Wno-interrupts_property + +# Apply overlays statically with fdtoverlay. This is a build time test that +# the overlays can be applied successfully by fdtoverlay. This does not +# guarantee that the overlays can be applied successfully at run time by +# unittest, but it provides a bit of build time test coverage for those +# who do not execute unittest. +# +# The overlays are applied on top of static_base_1.dtb and static_base_2.dtb to +# create static_test_1.dtb and static_test_2.dtb. If fdtoverlay detects an +# error than the kernel build will fail. static_test_1.dtb and +# static_test_2.dtb are not consumed by unittest. +# +# Some unittest overlays deliberately contain errors that unittest checks for. +# These overlays will cause fdtoverlay to fail, and are thus not included +# in the static test: +#overlay_bad_add_dup_node.dtb \ +#overlay_bad_add_dup_prop.dtb \ +#overlay_bad_phandle.dtb \ +#overlay_bad_symbol.dtb \ + +apply_static_overlay_1 := overlay_0.dtbo \ + overlay_1.dtbo \ + overlay_2.dtbo \ + overlay_3.dtbo \ + overlay_4.dtbo \ + overlay_5.dtbo \ + overlay_6.dtbo \ + overlay_7.dtbo \ + overlay_8.dtbo \ + overlay_9.dtbo \ + overlay_10.dtbo \ + overlay_11.dtbo \ + overlay_12.dtbo \ + overlay_13.dtbo \ + overlay_15.dtbo \ + overlay_gpio_01.dtbo \ + overlay_gpio_02a.dtbo \ + overlay_gpio_02b.dtbo \ + overlay_gpio_03.dtbo \ + overlay_gpio_04a.dtbo \ + overlay_gpio_04b.dtbo + +apply_static_overlay_2 := overlay.dtbo + +static_test_1-dtbs := static_base_1.dtb $(apply_static_overlay_1) +static_test_2-dtbs := static_base_2.dtb $(apply_static_overlay_2) + +overlay-$(CONFIG_OF_OVERLAY) += static_test_1.dtb static_test_2.dtb diff --git a/drivers/of/unittest-data/static_base_1.dts b/drivers/of/unittest-data/static_base_1.dts new file mode 100644 index ..10556cb3f01f --- /dev/null +++ b/drivers/of/unittest-data/static_base_1.dts @@ -0,0 +1,4 @@ +// SPDX-License-Identifier: GPL-2.0 +/dts-v1/; + +#include "testcases_common.dtsi" diff --git a/drivers/of/unittest-data/static_base_2.dts b/drivers/of/unittest-data/static_base_2.dts new file mode 100644 index ..b0ea9504d6f3 --- /dev/null +++ b/drivers/of/unittest-data/static_base_2.dts @@ -0,0 +1,4 @@ +// SPDX-License-Identifier: GPL-2.0 +/dts-v1/; + +#include "overlay_common.dtsi" -- 2.25.0.rc1.19.g042ed3e048af
[PATCH V7 2/3] of: unittest: Create overlay_common.dtsi and testcases_common.dtsi
In order to build-test the same unit-test files using fdtoverlay tool,
move the device nodes from the existing overlay_base.dts and
testcases_common.dts files to .dtsi counterparts. The .dts files now
include the new .dtsi files, resulting in exactly the same behavior as
earlier.
The .dtsi files can now be reused for compile time tests using
fdtoverlay (will be done by a later commit).
This is required because the base files passed to fdtoverlay tool
shouldn't be overlays themselves (i.e. shouldn't have the /plugin/;
tag).
Note that this commit also moves "testcase-device2" node to
testcases.dts from tests-interrupts.dtsi, as this node has a deliberate
error in it and is only relevant for runtime testing done with
unittest.c.
Signed-off-by: Viresh Kumar
---
drivers/of/unittest-data/overlay_base.dts | 90 +-
drivers/of/unittest-data/overlay_common.dtsi | 91 +++
drivers/of/unittest-data/testcases.dts| 18 ++--
.../of/unittest-data/testcases_common.dtsi| 19
.../of/unittest-data/tests-interrupts.dtsi| 7 --
5 files changed, 118 insertions(+), 107 deletions(-)
create mode 100644 drivers/of/unittest-data/overlay_common.dtsi
create mode 100644 drivers/of/unittest-data/testcases_common.dtsi
diff --git a/drivers/of/unittest-data/overlay_base.dts
b/drivers/of/unittest-data/overlay_base.dts
index 99ab9d12d00b..ab9014589c5d 100644
--- a/drivers/of/unittest-data/overlay_base.dts
+++ b/drivers/of/unittest-data/overlay_base.dts
@@ -2,92 +2,4 @@
/dts-v1/;
/plugin/;
-/*
- * Base device tree that overlays will be applied against.
- *
- * Do not add any properties in node "/".
- * Do not add any nodes other than "/testcase-data-2" in node "/".
- * Do not add anything that would result in dtc creating node "/__fixups__".
- * dtc will create nodes "/__symbols__" and "/__local_fixups__".
- */
-
-/ {
- testcase-data-2 {
- #address-cells = <1>;
- #size-cells = <1>;
-
- electric_1: substation@100 {
- compatible = "ot,big-volts-control";
- reg = < 0x0100 0x100 >;
- status = "disabled";
-
- hvac_1: hvac-medium-1 {
- compatible = "ot,hvac-medium";
- heat-range = < 50 75 >;
- cool-range = < 60 80 >;
- };
-
- spin_ctrl_1: motor-1 {
- compatible = "ot,ferris-wheel-motor";
- spin = "clockwise";
- rpm_avail = < 50 >;
- };
-
- spin_ctrl_2: motor-8 {
- compatible = "ot,roller-coaster-motor";
- };
- };
-
- rides_1: fairway-1 {
- #address-cells = <1>;
- #size-cells = <1>;
- compatible = "ot,rides";
- status = "disabled";
- orientation = < 127 >;
-
- ride@100 {
- #address-cells = <1>;
- #size-cells = <1>;
- compatible = "ot,roller-coaster";
- reg = < 0x0100 0x100 >;
- hvac-provider = < _1 >;
- hvac-thermostat = < 29 > ;
- hvac-zones = < 14 >;
- hvac-zone-names = "operator";
- spin-controller = < _ctrl_2 5 _ctrl_2
7 >;
- spin-controller-names = "track_1", "track_2";
- queues = < 2 >;
-
- track@30 {
- reg = < 0x0030 0x10 >;
- };
-
- track@40 {
- reg = < 0x0040 0x10 >;
- };
-
- };
- };
-
- lights_1: lights@3 {
- compatible = "ot,work-lights";
- reg = < 0x0003 0x1000 >;
- status = "disabled";
- };
-
- lights_2: lights@4 {
- compatible = "ot,show-lights";
- reg = < 0x0004 0x1000 >;
- status = "disabled";
- rate = < 13 138 >;
-
[PATCH V7 1/3] kbuild: Add generic rule to apply fdtoverlay
From: Rob Herring Add a generic rule to apply fdtoverlay in Makefile.lib, so every platform doesn't need to carry the complex rule. The platform's Makefile only needs to have this now: DTC_FLAGS_foo_base += -@ foo-dtbs := foo_base.dtb foo_overlay1.dtbo foo_overlay2.dtbo overlay-y := foo.dtb Rearrange Makefile.lib to keep DT specific stuff together. The files from overlay-y (i.e. files generated by fdtoverlay) aren't added to dtb-y here, as dtb-y is later used to generate .dt.yaml files and the files in overlay-y don't have a corresponding dts file and make dtbs_check fails for them. Signed-off-by: Rob Herring [ Viresh: Add commit log and replace dtb-y with overlay-y, handle CONFIG_OF_ALL_DTBS case, rearrange Makefile, don't add overlay-y to dtb-y to skip dtbs_check for them. ] Signed-off-by: Viresh Kumar --- scripts/Makefile.lib | 39 +++ 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index b00855b247e0..a6e79e3be527 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -66,23 +66,16 @@ multi-used := $(multi-used-y) $(multi-used-m) real-obj-y := $(foreach m, $(obj-y), $(if $(strip $($(m:.o=-objs)) $($(m:.o=-y)) $($(m:.o=-))),$($(m:.o=-objs)) $($(m:.o=-y)),$(m))) real-obj-m := $(foreach m, $(obj-m), $(if $(strip $($(m:.o=-objs)) $($(m:.o=-y)) $($(m:.o=-m)) $($(m:.o=-))),$($(m:.o=-objs)) $($(m:.o=-y)) $($(m:.o=-m)),$(m))) -always-y += $(always-m) - -# hostprogs-always-y += foo -# ... is a shorthand for -# hostprogs += foo -# always-y += foo -hostprogs += $(hostprogs-always-y) $(hostprogs-always-m) -always-y += $(hostprogs-always-y) $(hostprogs-always-m) - -# userprogs-always-y is likewise. -userprogs += $(userprogs-always-y) $(userprogs-always-m) -always-y += $(userprogs-always-y) $(userprogs-always-m) +# Add base dtb and overlay dtbo +dtb-y += $(foreach m,$(overlay-y), $(if $(strip $($(m:.dtb=-dtbs))),$($(m:.dtb=-dtbs)),)) +dtb-$(CONFIG_OF_ALL_DTBS) += $(foreach m,$(overlay-), $(if $(strip $($(m:.dtb=-dtbs))),$($(m:.dtb=-dtbs)),)) # DTB # If CONFIG_OF_ALL_DTBS is enabled, all DT blobs are built extra-y+= $(dtb-y) +extra-y+= $(overlay-y) extra-$(CONFIG_OF_ALL_DTBS)+= $(dtb-) +extra-$(CONFIG_OF_ALL_DTBS)+= $(overlay-) ifneq ($(CHECK_DTBS),) extra-y += $(patsubst %.dtb,%.dt.yaml, $(dtb-y)) @@ -91,6 +84,19 @@ extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtb,%.dt.yaml, $(dtb-)) extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtbo,%.dt.yaml, $(dtb-)) endif +always-y += $(always-m) + +# hostprogs-always-y += foo +# ... is a shorthand for +# hostprogs += foo +# always-y += foo +hostprogs += $(hostprogs-always-y) $(hostprogs-always-m) +always-y += $(hostprogs-always-y) $(hostprogs-always-m) + +# userprogs-always-y is likewise. +userprogs += $(userprogs-always-y) $(userprogs-always-m) +always-y += $(userprogs-always-y) $(userprogs-always-m) + # Add subdir path extra-y:= $(addprefix $(obj)/,$(extra-y)) @@ -332,6 +338,15 @@ $(obj)/%.dtb: $(src)/%.dts $(DTC) FORCE $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE $(call if_changed_dep,dtc) + +quiet_cmd_fdtoverlay = DTOVL $@ + cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $(real-prereqs) + +.SECONDEXPANSION: + +$(obj)/%.dtb: $$(addprefix $$(obj)/,$$(%-dtbs)) FORCE + $(call if_changed,fdtoverlay) + DT_CHECKER ?= dt-validate DT_BINDING_DIR := Documentation/devicetree/bindings # DT_TMP_SCHEMA may be overridden from Documentation/devicetree/bindings/Makefile -- 2.25.0.rc1.19.g042ed3e048af
[PATCH V7 0/3] dt: Build unittests statically with fdtoverlay
Hi, The first four patches from the previous patchset are already picked by Rob for 5.12. This patchset contains the other two that update unittests and an additional patch suggested by Rob. V6->V7: - Dropped the first 4 patches, already merged. - Patch 1/3 is new, suggested by Rob and slightly modified by me. - Adapt Patch 3/3 to the new rule and name the overlay dtbs as .dtbo. -- Viresh Rob Herring (1): kbuild: Add generic rule to apply fdtoverlay Viresh Kumar (2): of: unittest: Create overlay_common.dtsi and testcases_common.dtsi of: unittest: Statically apply overlays using fdtoverlay drivers/of/unittest-data/Makefile | 50 ++ drivers/of/unittest-data/overlay_base.dts | 90 +- drivers/of/unittest-data/overlay_common.dtsi | 91 +++ drivers/of/unittest-data/static_base_1.dts| 4 + drivers/of/unittest-data/static_base_2.dts| 4 + drivers/of/unittest-data/testcases.dts| 18 ++-- .../of/unittest-data/testcases_common.dtsi| 19 .../of/unittest-data/tests-interrupts.dtsi| 7 -- scripts/Makefile.lib | 39 +--- 9 files changed, 203 insertions(+), 119 deletions(-) create mode 100644 drivers/of/unittest-data/overlay_common.dtsi create mode 100644 drivers/of/unittest-data/static_base_1.dts create mode 100644 drivers/of/unittest-data/static_base_2.dts create mode 100644 drivers/of/unittest-data/testcases_common.dtsi -- 2.25.0.rc1.19.g042ed3e048af
Re: [PATCH Resend] mailbox: arm_mhuv2: Fix sparse warnings
On 10-02-21, 00:25, Jassi Brar wrote: > Yup any bug fix should be sent in rc. But this, imo, lies on the > boundary of code and cosmetic issues, so I practiced discretion to > keep it for the next pull request lest I won't have much to send ;) Fair enough, would have been better though if you could have replied with this earlier. I had no clue on what's going on until this email came from you :) Thanks anyway. -- viresh
Re: [PATCH v4 5/7] cpufreq: qcom-hw: Implement CPRh aware OSM programming
On 20-01-21, 13:05, Bjorn Andersson wrote: > On Wed 20 Jan 12:25 CST 2021, Taniya Das wrote: > > > The CPUFREQ-HW driver is intended to be used only for CPUFREQ HW designs > > where the firmware programs the look up tables. > > > > It's obvious that this is the intended target for the current version of > the driver, but what are your technical arguments for keeping it that > way? > > > Suggestion is to separate out the driver where the programming is managed by > > high level OS. > > > > Can you please elaborate on the benefits of this approach? > > PS. Please don't top-post on LKML. Taniya, Can you please respond back to this ? We are waiting for merging this patchset.. Bjorn, can you or someone else please review this patch ? -- viresh
[PATCH Resend] mailbox: arm_mhuv2: Fix sparse warnings
This patch fixes a bunch of sparse warnings in the newly added arm_mhuv2
driver.
drivers/mailbox/arm_mhuv2.c:506:24: warning: incorrect type in argument 1
(different address spaces)
drivers/mailbox/arm_mhuv2.c:506:24:expected void const volatile [noderef]
__iomem *addr
drivers/mailbox/arm_mhuv2.c:506:24:got unsigned int [usertype] *
drivers/mailbox/arm_mhuv2.c:547:42: warning: incorrect type in argument 2
(different address spaces)
drivers/mailbox/arm_mhuv2.c:547:42:expected unsigned int [usertype] *reg
drivers/mailbox/arm_mhuv2.c:547:42:got unsigned int [noderef] __iomem *
drivers/mailbox/arm_mhuv2.c:625:42: warning: incorrect type in argument 2
(different address spaces)
drivers/mailbox/arm_mhuv2.c:625:42:expected unsigned int [usertype] *reg
drivers/mailbox/arm_mhuv2.c:625:42:got unsigned int [noderef] __iomem *
drivers/mailbox/arm_mhuv2.c:972:24: warning: dereference of noderef expression
drivers/mailbox/arm_mhuv2.c:973:22: warning: dereference of noderef expression
drivers/mailbox/arm_mhuv2.c:993:25: warning: dereference of noderef expression
drivers/mailbox/arm_mhuv2.c:1026:24: warning: dereference of noderef expression
drivers/mailbox/arm_mhuv2.c:1027:22: warning: dereference of noderef expression
drivers/mailbox/arm_mhuv2.c:1048:17: warning: dereference of noderef expression
Reported-by: kernel test robot
Signed-off-by: Viresh Kumar
---
Hi,
This should have been merged to 5.11-rc since the driver got introduced
in 5.11-rc1 itself. I don't see it queued for linux-next as well. It was
posted over 6 weeks back and no response is received yet:
https://lore.kernel.org/lkml/db5dd593cfd8b428ce44c1cce7484d887fa5e67c.1609303304.git.viresh.ku...@linaro.org/
Would be good if this can still go in 5.11.
drivers/mailbox/arm_mhuv2.c | 22 +++---
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/mailbox/arm_mhuv2.c b/drivers/mailbox/arm_mhuv2.c
index 67fb10885bb4..8223c1005254 100644
--- a/drivers/mailbox/arm_mhuv2.c
+++ b/drivers/mailbox/arm_mhuv2.c
@@ -238,19 +238,19 @@ struct mhuv2_mbox_chan_priv {
};
/* Macro for reading a bitfield within a physically mapped packed struct */
-#define readl_relaxed_bitfield(_regptr, _field)
\
+#define readl_relaxed_bitfield(_regptr, _type, _field) \
({ \
u32 _regval;\
_regval = readl_relaxed((_regptr)); \
- (*(typeof((_regptr)))(&_regval))._field;\
+ (*(_type *)(&_regval))._field; \
})
/* Macro for writing a bitfield within a physically mapped packed struct */
-#define writel_relaxed_bitfield(_value, _regptr, _field) \
+#define writel_relaxed_bitfield(_value, _regptr, _type, _field)
\
({ \
u32 _regval;\
_regval = readl_relaxed(_regptr); \
- (*(typeof(_regptr))(&_regval))._field = _value; \
+ (*(_type *)(&_regval))._field = _value; \
writel_relaxed(_regval, _regptr); \
})
@@ -496,7 +496,7 @@ static const struct mhuv2_protocol_ops
mhuv2_data_transfer_ops = {
/* Interrupt handlers */
-static struct mbox_chan *get_irq_chan_comb(struct mhuv2 *mhu, u32 *reg)
+static struct mbox_chan *get_irq_chan_comb(struct mhuv2 *mhu, u32 __iomem *reg)
{
struct mbox_chan *chans = mhu->mbox.chans;
int channel = 0, i, offset = 0, windows, protocol, ch_wn;
@@ -969,8 +969,8 @@ static int mhuv2_tx_init(struct amba_device *adev, struct
mhuv2 *mhu,
mhu->mbox.ops = _sender_ops;
mhu->send = reg;
- mhu->windows = readl_relaxed_bitfield(>send->mhu_cfg, num_ch);
- mhu->minor = readl_relaxed_bitfield(>send->aidr, arch_minor_rev);
+ mhu->windows = readl_relaxed_bitfield(>send->mhu_cfg, struct
mhu_cfg_t, num_ch);
+ mhu->minor = readl_relaxed_bitfield(>send->aidr, struct aidr_t,
arch_minor_rev);
spin_lock_init(>doorbell_pending_lock);
@@ -990,7 +990,7 @@ static int mhuv2_tx_init(struct amba_device *adev, struct
mhuv2 *mhu,
mhu->mbox.txdone_poll = false;
mhu->irq = adev->irq[0];
- writel_relaxed_bitfield(1, >send->int_en, chcomb);
+ writel_relaxed_bitfield(1, >send->int_en, struct
int_en_t, chcomb);
/* Disable all channel interrupts */
for (i = 0; i < mhu->windows; i++)
@@ -1023,8 +1023,8 @@ static int mhuv2_rx_init(struct amba_devic
Re: [PATCH] cpufreq: schedutil: Don't use the limits_changed flag any more
On 08-02-21, 11:07, Yue Hu wrote:
> From: Yue Hu
>
> The limits_changed flag was introduced by commit 600f5badb78c
> ("cpufreq: schedutil: Don't skip freq update when limits change") due
> to race condition where need_freq_update is cleared in get_next_freq()
> which causes reducing the CPU frequency is ineffective while busy.
>
> But now, the race condition above is gone because get_next_freq()
> doesn't clear the flag any more after commit 23a881852f3e ("cpufreq:
> schedutil: Don't skip freq update if need_freq_update is set").
>
> Moreover, need_freq_update currently will be set to true only in
> sugov_should_update_freq() if CPUFREQ_NEED_UPDATE_LIMITS is not set
> for the driver. However, limits may have changed at any time. And
> subsequent frequence update is depending on need_freq_update. So, we
> may skip this update.
>
> Hence, let's remove it to avoid above issue and make code more simple.
>
> Signed-off-by: Yue Hu
> ---
> kernel/sched/cpufreq_schedutil.c | 11 +++
> 1 file changed, 3 insertions(+), 8 deletions(-)
Acked-by: Viresh Kumar
--
viresh
Re: [PATCH V7 6/6] of: unittest: Statically apply overlays using fdtoverlay
On 08-02-21, 08:21, Rob Herring wrote: > We may need to turn off > checks in overlays for required properties as they may be incomplete. > We already do that on disabled nodes. And after decent amount of effort understanding how to do this, I finally did it in a not so efficient way, I am sure you can help improving it :) Author: Viresh Kumar Date: Tue Feb 9 12:19:50 2021 +0530 dt-validate: Skip "required property" checks for overlays The overlays may not carry the required properties and would depend on the base dtb to carry those, there is no point raising those errors here. Signed-off-by: Viresh Kumar --- tools/dt-validate | 17 + 1 file changed, 17 insertions(+) diff --git a/tools/dt-validate b/tools/dt-validate index 410b0538ef47..c6117504f1d1 100755 --- a/tools/dt-validate +++ b/tools/dt-validate @@ -80,6 +80,23 @@ show_unmatched = False (filename, line, col, fullname, node['compatible']), file=sys.stderr) continue +if nodename == '/': +is_fragment = False +for name in node.items(): +if name[0] == 'fragment@0': +is_fragment = True +break; + +if is_fragment == True: +if 'required property' in error.message: +continue +elif error.context: +for e in error.context: +if not 'required property' in e.message: +break +else: +continue + print(dtschema.format_error(filename, error, nodename=nodename, verbose=verbose) + '\n\tFrom schema: ' + schema['$filename'], file=sys.stderr) -- viresh
Re: [PATCH V7 6/6] of: unittest: Statically apply overlays using fdtoverlay
On 08-02-21, 08:21, Rob Herring wrote:
> In string 'm' replace '.dtb' with '-dtbs'. Then we get the value of
> that variable.
Ah, thanks. I was able to get everything to work as expected now :)
> > ifneq ($(CHECK_DTBS),)
> > extra-y += $(patsubst %.dtb,%.dt.yaml, $(dtb-y))
> > +extra-y += $(patsubst %.dtbo,%.dt.yaml, $(dtb-y))
> > extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtb,%.dt.yaml, $(dtb-))
> > +extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtbo,%.dt.yaml, $(dtb-))
> > endif
>
> I'll have to try that out. I think that should work.
It works with your patch itself, just that it was done after the
failure and so wasn't happening.
> > 2. This fails dtbs_check as it tries to run it for the source file of
> >test1.dtb
> >
> > $ make ARCH=arm64 O=../barm64/ -j8 CROSS_COMPILE=aarch64-linux-gnu-
> > dtbs_check
> > make[1]: Entering directory '/mnt/ssd/all/work/repos/devel/barm64'
> > make[3]: *** No rule to make target
> > 'arch/arm64/boot/dts/hisilicon/test1.dt.yaml', needed by '__build'. Stop.
> > /mnt/ssd/all/work/repos/devel/linux/scripts/Makefile.build:496: recipe for
> > target 'arch/arm64/boot/dts/hisilicon' failed
> > make[2]: *** [arch/arm64/boot/dts/hisilicon] Error 2
> > make[2]: *** Waiting for unfinished jobs
> > /mnt/ssd/all/work/repos/devel/linux/Makefile:1345: recipe for target 'dtbs'
> > failed
> > make[1]: *** [dtbs] Error 2
> > make[1]: Leaving directory '/mnt/ssd/all/work/repos/devel/barm64'
> > Makefile:185: recipe for target '__sub-make' failed
> > make: *** [__sub-make] Error 2
> >
> > I am not sure how to fix this.
>
> Even if we fixed the make rules, it's not going to work with
> validation. There's some information from source files that we
> maintain in yaml output, but is lost in dtb output. For example, the
> sizes of /bits/ syntax are maintained. For now, I think we'll want to
> just validate base and overlays separately. We may need to turn off
> checks in overlays for required properties as they may be incomplete.
> We already do that on disabled nodes.
I did this instead and it made everything work, we don't try dt.yaml
for the test1.dtb file anymore, is this acceptable ?
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 886d2e6c58f0..b86ff1b3de14 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -66,7 +66,7 @@ multi-used := $(multi-used-y) $(multi-used-m)
real-obj-y := $(foreach m, $(obj-y), $(if $(strip $($(m:.o=-objs))
$($(m:.o=-y)) $($(m:.o=-))),$($(m:.o=-objs)) $($(m:.o=-y)),$(m)))
real-obj-m := $(foreach m, $(obj-m), $(if $(strip $($(m:.o=-objs))
$($(m:.o=-y)) $($(m:.o=-m)) $($(m:.o=-))),$($(m:.o=-objs)) $($(m:.o=-y))
$($(m:.o=-m)),$(m)))
-real-dtb-y := $(foreach m,$(dtb-y), $(if $(strip
$($(m:.dtb=-dtbs))),$($(m:.dtb=-dtbs)),))
+real-dtb-y := $(foreach m,$(overlay-y), $(if $(strip
$($(m:.dtb=-dtbs))),$($(m:.dtb=-dtbs)),))
dtb-y += $(real-dtb-y)
always-y += $(always-m)
diff --git a/arch/arm64/boot/dts/hisilicon/Makefile
b/arch/arm64/boot/dts/hisilicon/Makefile
index f4d68caeba83..69ca27014e89 100644
--- a/arch/arm64/boot/dts/hisilicon/Makefile
+++ b/arch/arm64/boot/dts/hisilicon/Makefile
@@ -6,3 +6,8 @@ dtb-$(CONFIG_ARCH_HISI) += hi6220-hikey.dtb
dtb-$(CONFIG_ARCH_HISI) += hip05-d02.dtb
dtb-$(CONFIG_ARCH_HISI) += hip06-d03.dtb
dtb-$(CONFIG_ARCH_HISI) += hip07-d05.dtb
+
+DTC_FLAGS_hi3660-hikey960 += -@
+
+test1-dtbs := hi3660-hikey960.dtb hi3660-hikey960-overlay.dtbo
+overlay-y += test1.dtb
diff --git a/arch/arm64/boot/dts/hisilicon/hi3660-hikey960-overlay.dts
b/arch/arm64/boot/dts/hisilicon/hi3660-hikey960-overlay.dts
new file mode 100644
index ..1235a911caae
--- /dev/null
+++ b/arch/arm64/boot/dts/hisilicon/hi3660-hikey960-overlay.dts
@@ -0,0 +1,16 @@
+// SPDX-License-Identifier: GPL-2.0
+/dts-v1/;
+/plugin/;
+
+ {
+ #address-cells = <0x1>;
+ #size-cells = <0x0>;
+
+ wlcore2: wlcore@5 {
+ compatible = "ti,wl1837";
+ reg = <2>;
+ interrupt-parent = <>;
+ interrupts = <3 1>;
+ test = <1>;
+ };
+};
--
viresh
