* Re: [PATCH] ARM: dts: rockchip: reuse firefly dtsi
From: Heiko Stuebner @ 2017-04-18 22:20 UTC (permalink / raw)
To: Eddie Cai
Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
linux-I+IVW8TIWO2tmTQ+vhA3Yw,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170418121527.3155-1-eddie.cai.linux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Hi Eddie,
Am Dienstag, 18. April 2017, 20:15:27 CEST schrieb Eddie Cai:
> firefly reload is very similar with firefly board, so reuse firefly dtsi
>
> Signed-off-by: Eddie Cai <eddie.cai.linux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> arch/arm/boot/dts/rk3288-firefly-reload-core.dtsi | 310 ------------------
> arch/arm/boot/dts/rk3288-firefly-reload.dts | 368 ++--------------------
I would disagree and remember having a similar discussion when the reload-
support was initially submitted. Please keep in mind that the firefly-
reload is a som+baseboard combination, so somebody could (or maybe
already has) create a completely different baseboard for the som that
does not have any similarities with the original firefly.
The previous firefly being a real single board.
We also don't combine rock2 and firefly and other boards following the
general rk3288 design guidelines :-) and the original firefly and reload are
very different boards if you look at them.
Heiko
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller
From: Peter Rosin @ 2017-04-18 21:53 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-kernel, Wolfram Sang, Rob Herring, Mark Rutland,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Jonathan Corbet, linux-i2c, devicetree,
linux-iio, linux-doc, Andrew Morton, Colin Ian King,
Paul Gortmaker, Philipp Zabel, kernel
In-Reply-To: <20170418114430.GA1496@kroah.com>
On 2017-04-18 13:44, Greg Kroah-Hartman wrote:
> On Tue, Apr 18, 2017 at 12:59:50PM +0200, Peter Rosin wrote:
>> On 2017-04-18 10:51, Greg Kroah-Hartman wrote:
>>> On Thu, Apr 13, 2017 at 06:43:07PM +0200, Peter Rosin wrote:
>>>> +config MUX_GPIO
>>>> + tristate "GPIO-controlled Multiplexer"
>>>> + depends on OF && GPIOLIB
>>>
>>> Why have the gpio and mux core in the same patch?
>>
>> One is not usable w/o the other. I can split them if you want to?
>
> Then why are they two different config options? Add the core code in
> one patch, and then add the gpio controled mutiplexer in a separate
> patch.
Ah, I meant when there are not yet any other mux drivers. I'll just
split the patch.
>>>> +static struct class mux_class = {
>>>> + .name = "mux",
>>>> + .owner = THIS_MODULE,
>>>> +};
>>>
>>> No Documentation/ABI/ update for your sysfs files? Please do so.
>>
>> Ok I'll look into it. Wasn't even aware that I added any. But there's the
>> new class of course...
>
> Hint, you have files, the devices that belong to the class :)
Right.
>>>> +static int __init mux_init(void)
>>>> +{
>>>> + return class_register(&mux_class);
>>>> +}
>>>> +
>>>> +static DEFINE_IDA(mux_ida);
>>>
>>> When your module is unloaded, you forgot to clean this structure up with
>>> what was done with it.
>>
>> I was under the impression that not providing an exit function for modules
>> made the module infrastructure prevent unloading (by bumping some reference
>> counter). Maybe that is a misconception?
>
> Ah, messy, don't do that. Make it so you can unload your module please,
> why wouldn't you want that to happen?
What made me do it was the worry that mux drivers might be left dangling
w/o the core. But that can probably only happen if someone is very
deliberately trying to break stuff by forcing unloads??
>>>> + mux_chip = kzalloc(sizeof(*mux_chip) +
>>>> + controllers * sizeof(*mux_chip->mux) +
>>>> + sizeof_priv, GFP_KERNEL);
>>>> + if (!mux_chip)
>>>> + return NULL;
>>>
>>> You don't return PTR_ERR(-ENOMEM)? Ok, why not? (I'm not arguing for
>>> it, just curious...)
>>
>> There's no particular reason. Do you think I should change it?
>
> What does the caller do with an error? Pass it up to where? Who gets
> it? Don't you want the caller to know you are out of memory?
The current callers return -ENOMEM when NULL is returned here. Looks
like I'm going to be doing some fairly major changes anyway so I'll
just change this too while at it...
>>>> +
>>>> + device_initialize(&mux_chip->dev);
>>>
>>> Why are you not registering the device here as well? Why have this be a
>>> two step process?
>>
>> Because of idle state handling. The drivers are expected to fill in
>> the desired idle state(s) after allocating the mux controller(s).
>> Then, when registering, the desired idle state is activated (if the
>> idle state is not idle-as-is, of course) and as a last step the mux
>> is "advertised".
>
> Ok, is that documented in the functions somewhere?
I'll make sure to add it if it's missing.
>>>> + ret = device_add(&mux_chip->dev);
>>>> + if (ret < 0)
>>>> + dev_err(&mux_chip->dev,
>>>> + "device_add failed in mux_chip_register: %d\n", ret);
>>>
>>> Did you run checkpatch.pl in strict mode on this new file? Please do so :)
>>
>> I did, and did it again just to be sure, and I do not get any complaints.
>> So, what's wrong?
>
> You list the function name in the printk string, it should complain
> that __func__ should be used. Oh well, it's just a perl script, it
> doesn't always catch everything.
> isn't always correct :)
Ah, ok.
>>>> +EXPORT_SYMBOL_GPL(devm_mux_chip_alloc);
>>>
>>>
>>> Having devm functions that create/destroy other struct devices worries
>>> me, do we have other examples of this happening today? Are you sure you
>>> got the reference counting all correct?
>>
>> drivers/iio/industrialio_core.c:devm_iio_device_alloc
>>
>> Or is the iio case different in some subtle way that I'm missing?
>
> I don't know, hopefully you got it all correct, I haven't audited that
> code path in a long time :)
Looks as ok to me as it did before. Moving on... :-)
>>>> +
>>>> +static int devm_mux_chip_match(struct device *dev, void *res, void *data)
>>>> +{
>>>> + struct mux_chip **r = res;
>>>> +
>>>> + if (WARN_ON(!r || !*r))
>>>
>>> How can this happen?
>>
>> It shouldn't. I copied the pattern from the iio subsystem.
>
> Then it should be removed there too...
Ok, I'll see if I can find time to suggest some patch(es) to Jonathan.
>>>> +void devm_mux_chip_free(struct device *dev, struct mux_chip *mux_chip)
>>>> +{
>>>> + WARN_ON(devres_release(dev, devm_mux_chip_release,
>>>> + devm_mux_chip_match, mux_chip));
>>>
>>> What can someone do with these WARN_ON() splats in the kernel log?
>>
>> Don't know. Again, I copied the pattern from the iio subsystem.
>
> If you don't know what it should be used for, don't copy it!
>
> Cargo-cult coding is horrible, please no.
Ok, I'll just drop those WARNs...
>>>> + /* ...or it's just contended. */
>>>> + down_write(&mux->lock);
>>>
>>> Why use a read/write lock at all? Have you tested this to verify it
>>> really is faster and needed?
>>
>> For one of the HW configuration that drove the development, the same mux
>> controller is used to mux both an I2C channel and a couple of ADC lines.
>>
>> If there is no kind of reader/writer locking going on, there is no way to
>> do ADC readings concurrently with an I2C transfer even when the consumers
>> want the mux in the same position. With an ordinary mutex controlling the
>> mux position, the consumers will unconditionally get serialized, which
>> seems like a waste to me. Or maybe I'm missing something?
>
> Why is serializing things a "waste"? Again, a rw semaphore is slower,
> takes more logic to get correct, and is very complex at times. If you
> are not SURE you need it, and that it matters, don't use it. And if you
> do use it, document the heck out of it how you need it and why.
It's a waste of time because two independent mux consumers of the same
mux controller can't do things concurrently even if they want the same
thing from the mux. Let's say that one mux consumer is an iio-mux and
one is an i2c-mux. Also, let's say that you for some reason need to get
a lot of samples at a determined rate through the iio-mux. With a mutex,
that can't happen if there is an access through the i2c-mux taking
"forever" in the eyes of the ADC/iio-mux, even if they both want the
shared mux controller to be in the same position. The ADC/iio-mux and
the i2c-mux would be waiting for each other for no purpose at all.
>>>> +
>>>> + if (mux->cached_state == state) {
>>>> + /*
>>>> + * Hmmm, someone else changed the mux to my liking.
>>>> + * That makes me wonder how long I waited for nothing?
>>>> + */
>>>> + downgrade_write(&mux->lock);
>>>
>>> Oh that always scares me... Are you _sure_ this is correct? And
>>> needed?
>>
>> It might not be needed, and it would probably work ok to just fall
>> through and call mux_control_set unconditionally. What is it that
>> always scares you exactly? Relying on cached state to be correct?
>> Downgrading writer locks?
>
> downgrading a writer lock scares me, especially for something as
> "simple" as this type of interface. Again, don't use it unless you
> _have_ to. Simple is good, start with that always.
Some kind of lock needs to be grabbed in mux_control_select and
released in mux_control_deselect that fixes the mux state while the
mux consumer goes about its business. For the reasons stated above I
went with a reader/writer lock instead of the mutex I had originally.
Agreed, the code in mux_control_select is a few more lines than I
like, but I suspected the big issue to be holding *any* lock over
a pair of "global" functions. Changing from holding a rw-lock as
reader to instead holding a mutex changes very little in my eyes.
mux_control_select is simply not *that* complicated...
>>>> + if (mux->idle_state != MUX_IDLE_AS_IS &&
>>>> + mux->idle_state != mux->cached_state)
>>>> + ret = mux_control_set(mux, mux->idle_state);
>>>> +
>>>> + up_read(&mux->lock);
>>>
>>> You require a lock to be held for a "global" function? Without
>>> documentation? Or even a sparse marking? That's asking for trouble...
>>
>> Documentation I can handle, but where should I look to understand how I
>> should add sparse markings?
>
> Run sparse on the code and see what it says :)
Will do.
>> The mux needs to be locked somehow. But as I stated in the cover letter
>> the rwsem isn't a perfect fit.
>>
>> I'm using an rwsem to lock a mux, but that isn't really a
>> perfect fit. Is there a better locking primitive that I don't
>> know about that fits better? I had a mutex at one point, but
>> that didn't allow any concurrent accesses at all. At least
>> the rwsem allows concurrent access as long as all users
>> agree on the mux state, but I suspect that the rwsem will
>> degrade to the mutex situation pretty quickly if there is
>> any contention.
>>
>> Also, the lock doesn't add anything if there is only one consumer of
>> a mux controller. Maybe there should be some mechanism for shortcutting
>> the locking for the (more common?) single-consumer case?
>>
>> But again, I need the locking for my multi-consumer use case.
>
> Go back to a mutex, and having a function that requires it to be held
> is, icky.
But how do you propose that the ickyness is avoided? It's a requirement
that any waiters are released when the mux is available...
*thinking/coding for a bit*
I'm going to experiment with the below (untested) code which AFAICT
has the issues that starvation is possible, that it isn't first-come
first-serve, and that once there is contention the waiters may wait
for a longer time than needed...
On the positive side there are no actual locks held from select over
to deselect and there's no rwsem. But a missing (or an extra) deselect
still messes things up pretty fatally, since the below is really just
some kind of open coded locking thingy that I thought was one of the
things I should stay away from. But maybe you like it better?
(In the below, mux->wait_sem is assumed to be initialized to zero.)
int mux_control_select(struct mux_control *mux, int state)
{
int ret = 0;
again:
mutex_lock(&mux->lock);
if (mux->cached_state == state) {
/* The mux is already correct, just bump the user count. */
++mux->users;
goto done;
}
if (mux->users) {
/* The mux needs updating but is in use, wait... */
++mux->waiters;
mutex_unlock(&mux->lock);
down(&mux->wait_sem);
goto again;
}
/* The mux needs updating and is unused. */
ret = mux_control_set(mux, state);
if (ret >= 0) {
++mux->users;
goto done;
}
/* The mux update failed, try to revert if appropriate... */
if (mux->idle_state != MUX_IDLE_AS_IS)
mux_control_set(mux, mux->idle_state);
/* ...and release a waiter if there is one. */
if (mux->waiters) {
--mux->waiters;
up(&mux->wait_sem);
}
done:
mutex_unlock(&mux->lock);
return ret;
}
int mux_control_deselect(struct mux_control *mux)
{
int ret = 0;
mutex_lock(&mux->lock);
if (--mux->users)
goto done;
/* This was the last user, idle the mux if appropriate... */
if (mux->idle_state != MUX_IDLE_AS_IS &&
mux->idle_state != mux->cached_state)
ret = mux_control_set(mux, mux->idle_state);
/* ...and release a waiter if there is one. */
if (mux->waiters) {
--mux->waiters;
up(&mux->wait_sem);
}
done:
mutex_unlock(&mux->lock);
return ret;
}
>>>> +/*
>>>> + * Using subsys_initcall instead of module_init here to ensure - for the
>>>> + * non-modular case - that the subsystem is initialized when mux consumers
>>>> + * and mux controllers start to use it /without/ relying on link order.
>>>> + * For the modular case, the ordering is ensured with module dependencies.
>>>> + */
>>>> +subsys_initcall(mux_init);
>>>
>>> Even with subsys_initcall you are relying on link order, you do realize
>>> that? What about other subsystems that rely on this? :)
>>
>> Yes, that is true, but if others start relying on this, that's their problem,
>> right? :-)
>
> Yes, but no need to document something that isn't true. You are relying
> on link order here.
Well, am I? If I change it to module_init I do get runtime errors for a
non-modular build (when mux consumers get initialized before the mux core).
With subsys_init the mux core get initialized before all (current)
consumers. I don't really see how the link order *currently* matters?
So, just making sure that we are on the same page, the thing that relies on
link order are any mux consumers/drivers that in the future may be added as
part of some other subsys_init call (or earlier). Right?
Hmmm, or are you perhaps referring to the fact that the mux core depends on
other subsystems being initialized first?
>>>> +struct mux_control_ops {
>>>> + int (*set)(struct mux_control *mux, int state);
>>>> +};
>>>> +
>>>> +/* These defines match the constants from the dt-bindings. On purpose. */
>>>
>>> Why on purpose?
>>
>> I sure wasn't an accident? :-)
>>
>> Want me to remove it?
>
> At least explain _why_ you are doing this, that would help, right?
Should I perhaps just #include <linux/dt-bindings/mux/mux.h> instead?
Is that an OK thing to do? I didn't because I feared it might come back
to haunt me at some point if the bindings header needed stuff in the
future that made it incompatible...
It's also not terribly common to include bindings from an include...
Cheers,
peda
^ permalink raw reply
* Re: [PATCH 0/4] of: remove *phandle properties from expanded device tree
From: Frank Rowand @ 2017-04-18 21:08 UTC (permalink / raw)
To: Rob Herring, Stephen Boyd
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <58F19A75.80500-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Hi Rob,
Please do not apply this patch series.
The more context I look at, the less this approach seems good.
I hope to have a simpler version completed quickly.
Thanks,
- Frank
On 04/14/17 20:58, Frank Rowand wrote:
> Hi Stephen,
>
> I left you off the distribution list, sorry...
>
> On 04/14/17 20:55, frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
>> From: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
>>
>> Remove "phandle" and "linux,phandle" properties from the internal
>> device tree. The phandle will still be in the struct device_node
>> phandle field.
>>
>> This is to resolve the issue found by Stephen Boyd [1] when he changed
>> the type of struct property.value from void * to const void *. As
>> a result of the type change, the overlay code had compile errors
>> where the resolver updates phandle values.
>>
>> [1] http://lkml.iu.edu/hypermail/linux/kernel/1702.1/04160.html
>>
>> Patch 1 is the phandle related changes.
>>
>> Patches 2 - 4 are minor fixups for issues that became visible
>> while implementing patch 1.
>>
>> Frank Rowand (4):
>> of: remove *phandle properties from expanded device tree
>> of: make __of_attach_node() static
>> of: be consistent in form of file mode
>> of: detect invalid phandle in overlay
>>
>> drivers/of/base.c | 53 +++++++++++++++++++++++++++++++++++++++++++++----
>> drivers/of/dynamic.c | 31 ++++++++++++++++-------------
>> drivers/of/fdt.c | 40 ++++++++++++++++++++++---------------
>> drivers/of/of_private.h | 1 -
>> drivers/of/overlay.c | 8 +++++---
>> drivers/of/resolver.c | 23 +--------------------
>> include/linux/of.h | 1 +
>> 7 files changed, 97 insertions(+), 60 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 2/2] hwmon: (brcmstb) Add driver for Broadcom STB DPFE
From: Guenter Roeck @ 2017-04-18 20:58 UTC (permalink / raw)
To: Markus Mayer
Cc: Jean Delvare, Rob Herring, Mark Rutland, Florian Fainelli,
Markus Mayer, Broadcom Kernel List, Linux HWMON List,
Device Tree List, ARM Kernel List, Linux Kernel Mailing List
In-Reply-To: <20170418201702.57019-3-code@mmayer.net>
Hi Markus,
On Tue, Apr 18, 2017 at 01:17:02PM -0700, Markus Mayer wrote:
> From: Markus Mayer <mmayer@broadcom.com>
>
> This driver allows access to DRAM properties, such as the refresh rate,
> via the Broadcom STB DDR PHY Front End (DPFE). The refresh rate can be
> used as indirect indicator of the DRAM temperature.
>
> The driver also allows setting of the sampling interval.
>
> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> ---
[ ... ]
> +
> +static SENSOR_DEVICE_ATTR(dpfe_info, 0444, show_info, NULL, 1000);
> +static SENSOR_DEVICE_ATTR(dpfe_refresh, 0644, show_refresh, store_refresh,
> + 1000);
> +static SENSOR_DEVICE_ATTR(dpfe_vendor, 0444, show_vendor, NULL, 1000);
> +static struct attribute *dpfe_attrs[] = {
> + &sensor_dev_attr_dpfe_info.dev_attr.attr,
> + &sensor_dev_attr_dpfe_refresh.dev_attr.attr,
> + &sensor_dev_attr_dpfe_vendor.dev_attr.attr,
> + NULL
> +};
> +ATTRIBUTE_GROUPS(dpfe);
> +
There is not a single standard hwmon attribute. I don't know how
to classify this driver, and where it should reside, but it is not
a hwmon driver.
Thanks,
Guenter
^ permalink raw reply
* [PATCH 2/2] hwmon: (brcmstb) Add driver for Broadcom STB DPFE
From: Markus Mayer @ 2017-04-18 20:17 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, Rob Herring, Mark Rutland,
Florian Fainelli
Cc: Markus Mayer, Broadcom Kernel List, Linux HWMON List,
Device Tree List, ARM Kernel List, Linux Kernel Mailing List
In-Reply-To: <20170418201702.57019-1-code@mmayer.net>
From: Markus Mayer <mmayer@broadcom.com>
This driver allows access to DRAM properties, such as the refresh rate,
via the Broadcom STB DDR PHY Front End (DPFE). The refresh rate can be
used as indirect indicator of the DRAM temperature.
The driver also allows setting of the sampling interval.
Signed-off-by: Markus Mayer <mmayer@broadcom.com>
---
MAINTAINERS | 8 +
drivers/hwmon/Kconfig | 13 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/brcmstb-dpfe.c | 689 +++++++++++++++++++++++++++++++++++++++++++
4 files changed, 711 insertions(+)
create mode 100644 drivers/hwmon/brcmstb-dpfe.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 676c139..d2d0495 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2825,6 +2825,14 @@ S: Maintained
F: Documentation/devicetree/bindings/cpufreq/brcm,stb-avs-cpu-freq.txt
F: drivers/cpufreq/brcmstb*
+BROADCOM STB DPFE HWMON DRIVER
+M: Markus Mayer <mmayer@broadcom.com>
+M: bcm-kernel-feedback-list@broadcom.com
+L: linux-hwmon@vger.kernel.org
+S: Maintained
+F: Documentation/devicetree/bindings/hwmon/brcmstb*
+F: drivers/hwmon/brcmstb*
+
BROADCOM SPECIFIC AMBA DRIVER (BCMA)
M: Rafał Miłecki <zajec5@gmail.com>
L: linux-wireless@vger.kernel.org
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 0649d53f3..54bb55c 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -355,6 +355,19 @@ config SENSORS_ATXP1
This driver can also be built as a module. If so, the module
will be called atxp1.
+config SENSORS_BRCMSTB_DPFE
+ tristate "BRCMSTB DPFE driver"
+ depends on ARCH_BRCMSTB
+ default y
+ help
+ If you say yes here you get support for the Broadcom set-top box
+ DDR PHY Front End (DPFE) interface.
+
+ The driver allows one to query the DRAM refresh rate, which in turn,
+ provides an indirect means for deriving the DRAM temperature.
+
+ If in doubt, say N.
+
config SENSORS_DS620
tristate "Dallas Semiconductor DS620"
depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 5509edf..cec13ec 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_SENSORS_ATK0110) += asus_atk0110.o
# Native drivers
# asb100, then w83781d go first, as they can override other drivers' addresses.
obj-$(CONFIG_SENSORS_ASB100) += asb100.o
+obj-$(CONFIG_SENSORS_BRCMSTB_DPFE) += brcmstb-dpfe.o
obj-$(CONFIG_SENSORS_W83627HF) += w83627hf.o
obj-$(CONFIG_SENSORS_W83792D) += w83792d.o
obj-$(CONFIG_SENSORS_W83793) += w83793.o
diff --git a/drivers/hwmon/brcmstb-dpfe.c b/drivers/hwmon/brcmstb-dpfe.c
new file mode 100644
index 0000000..01abea8
--- /dev/null
+++ b/drivers/hwmon/brcmstb-dpfe.c
@@ -0,0 +1,689 @@
+/*
+ * Temperature sensor driver for Broadcom set top box SoCs
+ *
+ * Copyright (c) 2017 Broadcom
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/delay.h>
+#include <linux/firmware.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+
+#define DRVNAME "brcmstb-dpfe"
+#define FIRMWARE_NAME "dpfe.bin"
+#define DT_COMPAT_DMEM "brcm,dpfe-dmem"
+#define DT_COMPAT_IMEM "brcm,dpfe-imem"
+
+/* DCPU register offsets */
+#define REG_DCPU_RESET 0x0
+#define REG_TO_DCPU_MBOX 0x10
+#define REG_TO_HOST_MBOX 0x14
+
+/* Message RAM */
+#define DCPU_MSG_RAM(x) (0x100 + (x) * sizeof(u32))
+
+/* DRAM Info Offsets & Masks */
+#define DRAM_INFO_INTERVAL 0x0
+#define DRAM_INFO_MR4 0x4
+#define DRAM_INFO_ERROR 0x8
+#define DRAM_INFO_MASK 0xff
+
+/* DRAM MR4 Offsets & Masks */
+#define DRAM_MR4_REFRESH 0x0 /* Refresh rate */
+#define DRAM_MR4_SR_ABORT 0x3 /* Self Refresh Abort */
+#define DRAM_MR4_PPRE 0x4 /* Post-package repair entry/exit */
+#define DRAM_MR4_TH_OFFS 0x5 /* Thermal Offset; vendor specific */
+#define DRAM_MR4_TUF 0x7 /* Temperature Update Flag */
+
+#define DRAM_MR4_REFRESH_MASK 0x7
+#define DRAM_MR4_SR_ABORT_MASK 0x1
+#define DRAM_MR4_PPRE_MASK 0x1
+#define DRAM_MR4_TH_OFFS_MASK 0x3
+#define DRAM_MR4_TUF_MASK 0x1
+
+/* DRAM Vendor Offsets & Masks */
+#define DRAM_VENDOR_MR5 0x0
+#define DRAM_VENDOR_MR6 0x4
+#define DRAM_VENDOR_MR7 0x8
+#define DRAM_VENDOR_MR8 0xc
+#define DRAM_VENDOR_ERROR 0x10
+#define DRAM_VENDOR_MASK 0xff
+
+/* Reset register bits & masks */
+#define DCPU_RESET_SHIFT 0x0
+#define DCPU_RESET_MASK 0x1
+#define DCPU_CLK_DISABLE_SHIFT 0x2
+
+/* DCPU return codes */
+#define DCPU_RET_SUCCESS 0x00000001
+#define DCPU_RET_ERR_HEADER 0x80000001
+#define DCPU_RET_ERR_INVAL 0x80000002
+#define DCPU_RET_ERR_CHKSUM 0x80000004
+#define DCPU_RET_ERR_OTHER 0x80000008
+
+/* Firmware magic */
+#define DPFE_BE_MAGIC 0xfe1010fe
+#define DPFE_LE_MAGIC 0xfe0101fe
+
+/* Error codes */
+#define ERR_INVALID_MAGIC -1
+#define ERR_INVALID_SIZE -2
+#define ERR_INVALID_CHKSUM -3
+
+/* Message types */
+#define DPFE_MSG_TYPE_COMMAND 1
+#define DPFE_MSG_TYPE_RESPONSE 2
+
+#define DELAY_LOOP_MAX 200000
+
+enum dpfe_msg_fields {
+ MSG_HEADER,
+ MSG_COMMAND,
+ MSG_ARG_COUNT,
+ MSG_ARG0,
+ MSG_CHKSUM,
+ MSG_FIELD_MAX /* Last entry */
+};
+
+enum dpfe_commands {
+ DPFE_CMD_GET_INFO,
+ DPFE_CMD_GET_REFRESH,
+ DPFE_CMD_GET_VENDOR,
+ DPFE_CMD_MAX /* Last entry */
+};
+
+struct dpfe_msg {
+ u32 header;
+ u32 command;
+ u32 arg_count;
+ u32 arg0;
+ u32 chksum; /* This is the sum of all other entries. */
+};
+
+/*
+ * Format of the binary firmware file:
+ *
+ * entry
+ * 0 header
+ * value: 0xfe0101fe <== little endian
+ * 0xfe1010fe <== big endian
+ * 1 sequence:
+ * [31:16] total segments on this build
+ * [15:0] this segment sequence.
+ * 2 FW version
+ * 3 IMEM byte size
+ * 4 DMEM byte size
+ * IMEM
+ * DMEM
+ * last checksum ==> sum of everything
+ */
+struct dpfe_firmware_header {
+ u32 magic;
+ u32 sequence;
+ u32 version;
+ u32 imem_size;
+ u32 dmem_size;
+};
+
+/* Things we only need during initialization. */
+struct init_data {
+ void __iomem *dmem;
+ void __iomem *imem;
+ unsigned int dmem_len;
+ unsigned int imem_len;
+ unsigned int chksum;
+ bool is_big_endian;
+};
+
+/* Things we need for as long as we are active. */
+struct private_data {
+ void __iomem *regs;
+ void __iomem *dmem;
+ struct mutex lock;
+};
+
+/* List of supported firmware commands */
+const u32 dpfe_commands[DPFE_CMD_MAX][MSG_FIELD_MAX] = {
+ [DPFE_CMD_GET_INFO] = {
+ [MSG_HEADER] = DPFE_MSG_TYPE_COMMAND,
+ [MSG_COMMAND] = 1,
+ [MSG_ARG_COUNT] = 1,
+ [MSG_ARG0] = 1,
+ [MSG_CHKSUM] = 4,
+ },
+ [DPFE_CMD_GET_REFRESH] = {
+ [MSG_HEADER] = DPFE_MSG_TYPE_COMMAND,
+ [MSG_COMMAND] = 2,
+ [MSG_ARG_COUNT] = 1,
+ [MSG_ARG0] = 1,
+ [MSG_CHKSUM] = 5,
+ },
+ [DPFE_CMD_GET_VENDOR] = {
+ [MSG_HEADER] = DPFE_MSG_TYPE_COMMAND,
+ [MSG_COMMAND] = 2,
+ [MSG_ARG_COUNT] = 1,
+ [MSG_ARG0] = 2,
+ [MSG_CHKSUM] = 6,
+ },
+};
+
+static u32 dpfe_readl(const void __iomem *addr)
+{
+ return le32_to_cpu(readl_relaxed(addr));
+}
+
+static void dpfe_writel(u32 value, void __iomem *addr)
+{
+ writel_relaxed(cpu_to_le32(value), addr);
+}
+
+static void __iomem *__map_region(const char *name)
+{
+ struct device_node *np;
+ void __iomem *ptr;
+
+ np = of_find_compatible_node(NULL, NULL, name);
+ if (!np)
+ return NULL;
+
+ ptr = of_iomap(np, 0);
+ of_node_put(np);
+
+ return ptr;
+}
+
+static void __disable_dcpu(void __iomem *regs)
+{
+ u32 val;
+
+ /* Check if DCPU is running */
+ val = dpfe_readl(regs + REG_DCPU_RESET);
+ if (!(val & DCPU_RESET_MASK)) {
+ /* Put DCPU in reset */
+ val |= (1 << DCPU_RESET_SHIFT);
+ dpfe_writel(val, regs + REG_DCPU_RESET);
+ }
+}
+
+static void __enable_dcpu(void __iomem *regs)
+{
+ u32 val;
+
+ /* Clear mailbox registers. */
+ dpfe_writel(0, regs + REG_TO_DCPU_MBOX);
+ dpfe_writel(0, regs + REG_TO_HOST_MBOX);
+
+ /* Disable DCPU clock gating */
+ val = dpfe_readl(regs + REG_DCPU_RESET);
+ val &= ~(1 << DCPU_CLK_DISABLE_SHIFT);
+ dpfe_writel(val, regs + REG_DCPU_RESET);
+
+ /* Take DCPU out of reset */
+ val = dpfe_readl(regs + REG_DCPU_RESET);
+ val &= ~(1 << DCPU_RESET_SHIFT);
+ dpfe_writel(val, regs + REG_DCPU_RESET);
+}
+
+static unsigned int get_msg_chksum(const u32 msg[])
+{
+ unsigned int sum = 0;
+ unsigned int i;
+
+ /* Don't include the last field in the checksum. */
+ for (i = 0; i < MSG_FIELD_MAX - 1; i++)
+ sum += msg[i];
+
+ return sum;
+}
+
+static int __send_command(struct private_data *priv, unsigned int cmd,
+ u32 result[])
+{
+ const u32 *msg = dpfe_commands[cmd];
+ void __iomem *regs = priv->regs;
+ unsigned int i, chksum;
+ int ret = 0;
+ u32 resp;
+
+ if (cmd >= DPFE_CMD_MAX)
+ return -1;
+
+ mutex_lock(&priv->lock);
+
+ /* Write command and arguments to message area */
+ for (i = 0; i < MSG_FIELD_MAX; i++)
+ dpfe_writel(msg[i], regs + DCPU_MSG_RAM(i));
+
+ /* Tell DCPU there is a command waiting */
+ dpfe_writel(1, regs + REG_TO_DCPU_MBOX);
+
+ /* Wait for DCPU to process the command */
+ for (i = 0; i < DELAY_LOOP_MAX; i++) {
+ /* Read response code */
+ resp = dpfe_readl(regs + REG_TO_HOST_MBOX);
+ if (resp > 0)
+ break;
+ udelay(5);
+ }
+ if (i == DELAY_LOOP_MAX)
+ ret = -ETIMEDOUT;
+
+ /* Read response data */
+ for (i = 0; i < MSG_FIELD_MAX; i++)
+ result[i] = dpfe_readl(regs + DCPU_MSG_RAM(i));
+
+ /* Tell DCPU we are done */
+ dpfe_writel(0, regs + REG_TO_HOST_MBOX);
+
+ mutex_unlock(&priv->lock);
+
+ if (!ret) {
+ /* Verify response */
+ chksum = get_msg_chksum(result);
+ if (chksum != result[MSG_CHKSUM])
+ ret = -1;
+ }
+
+ if (!ret) {
+ switch (resp) {
+ case DCPU_RET_SUCCESS:
+ break;
+ case DCPU_RET_ERR_HEADER:
+ case DCPU_RET_ERR_INVAL:
+ case DCPU_RET_ERR_CHKSUM:
+ case DCPU_RET_ERR_OTHER:
+ ret = -1;
+ break;
+ }
+ }
+
+ return ret;
+}
+
+/* Ensure that the firmware file loaded meets all the requirements. */
+static int __verify_firmware(struct init_data *init,
+ const struct firmware *fw)
+{
+ const struct dpfe_firmware_header *header = (void *)fw->data;
+ unsigned int dmem_size, imem_size, total_size;
+ bool is_big_endian = false;
+ const u32 *chksum;
+
+ if (header->magic == DPFE_BE_MAGIC)
+ is_big_endian = true;
+ else if (header->magic != DPFE_LE_MAGIC)
+ return ERR_INVALID_MAGIC;
+
+ if (is_big_endian) {
+ dmem_size = be32_to_cpu(header->dmem_size);
+ imem_size = be32_to_cpu(header->imem_size);
+ } else {
+ dmem_size = header->dmem_size;
+ imem_size = header->imem_size;
+ }
+
+ /* Data and instruction sections are 32 bit words. */
+ if ((dmem_size % sizeof(u32)) != 0 || (imem_size % sizeof(u32)) != 0)
+ return ERR_INVALID_SIZE;
+
+ /*
+ * The header + the data section + the instruction section + the
+ * checksum must be equal to the total firmware size.
+ */
+ total_size = dmem_size + imem_size + sizeof(*header) + sizeof(*chksum);
+ if (total_size != fw->size)
+ return ERR_INVALID_SIZE;
+
+ /* The checksum comes at the very end. */
+ chksum = (void *)fw->data + sizeof(*header) + dmem_size + imem_size;
+
+ init->is_big_endian = is_big_endian;
+ init->dmem_len = dmem_size;
+ init->imem_len = imem_size;
+ init->chksum = (is_big_endian) ? be32_to_cpu(*chksum) : *chksum;
+
+ return 0;
+}
+
+/* Verify checksum by reading back the firmware from co-processor RAM. */
+static int __verify_fw_checksum(struct init_data *init,
+ const struct dpfe_firmware_header *header,
+ u32 checksum)
+{
+ u32 magic, sequence, version, sum;
+ u32 __iomem *dmem = init->dmem;
+ u32 __iomem *imem = init->imem;
+ unsigned int i;
+
+ if (init->is_big_endian) {
+ magic = be32_to_cpu(header->magic);
+ sequence = be32_to_cpu(header->sequence);
+ version = be32_to_cpu(header->version);
+ } else {
+ magic = header->magic;
+ sequence = header->sequence;
+ version = header->version;
+ }
+
+ sum = magic + sequence + version + init->dmem_len + init->imem_len;
+
+ for (i = 0; i < init->dmem_len / sizeof(u32); i++)
+ sum += dpfe_readl(dmem + i);
+
+ for (i = 0; i < init->imem_len / sizeof(u32); i++)
+ sum += dpfe_readl(imem + i);
+
+ return (sum == checksum) ? 0 : -1;
+}
+
+static int __write_firmware(u32 __iomem *mem, const u32 *fw,
+ unsigned int size, bool is_big_endian)
+{
+ unsigned int i;
+
+ /* Convert size to 32-bit words. */
+ size /= sizeof(u32);
+
+ /* It is recommended to clear the firmware area first. */
+ for (i = 0; i < size; i++)
+ dpfe_writel(0, mem + i);
+
+ /* Now copy it. */
+ if (is_big_endian) {
+ for (i = 0; i < size; i++)
+ dpfe_writel(be32_to_cpu(fw[i]), mem + i);
+ } else {
+ for (i = 0; i < size; i++)
+ dpfe_writel(fw[i], mem + i);
+ }
+
+ return 0;
+}
+
+static int brcmstb_hwmon_download_firwmare(struct platform_device *pdev,
+ struct init_data *init)
+{
+ const struct dpfe_firmware_header *header;
+ unsigned int dmem_size, imem_size;
+ struct device *dev = &pdev->dev;
+ bool is_big_endian = false;
+ struct private_data *priv;
+ const struct firmware *fw;
+ const u32 *dmem, *imem;
+ const void *fw_blob;
+ int ret;
+
+ ret = request_firmware(&fw, FIRMWARE_NAME, dev);
+ /* request_firmware() prints its own error messages. */
+ if (ret)
+ return ret;
+
+ priv = platform_get_drvdata(pdev);
+
+ ret = __verify_firmware(init, fw);
+ if (ret)
+ return -EFAULT;
+
+ __disable_dcpu(priv->regs);
+
+ is_big_endian = init->is_big_endian;
+ dmem_size = init->dmem_len;
+ imem_size = init->imem_len;
+
+ /* At the beginning of the firmware blob is a header. */
+ header = (struct dpfe_firmware_header *)fw->data;
+ /* Void pointer to the beginning of the actual firmware. */
+ fw_blob = fw->data + sizeof(*header);
+ /* IMEM comes right after the header. */
+ imem = fw_blob;
+ /* DMEM follows after IMEM. */
+ dmem = fw_blob + imem_size;
+
+ ret = __write_firmware(init->dmem, dmem, dmem_size, is_big_endian);
+ if (ret)
+ return ret;
+ ret = __write_firmware(init->imem, imem, imem_size, is_big_endian);
+ if (ret)
+ return ret;
+
+ ret = __verify_fw_checksum(init, header, init->chksum);
+ if (ret)
+ return ret;
+
+ __enable_dcpu(priv->regs);
+
+ return 0;
+}
+
+static ssize_t generic_show(unsigned int command, u32 response[],
+ struct device *dev, char *buf)
+{
+ struct private_data *priv;
+ int ret;
+
+ priv = dev_get_drvdata(dev);
+
+ ret = __send_command(priv, command, response);
+ if (ret)
+ return sprintf(buf, "error %d\n", ret);
+
+ return 0;
+}
+
+static ssize_t show_info(struct device *dev, struct device_attribute *devattr,
+ char *buf)
+{
+ u32 response[MSG_FIELD_MAX];
+ unsigned int info;
+ int ret;
+
+ ret = generic_show(DPFE_CMD_GET_INFO, response, dev, buf);
+ if (ret)
+ return ret;
+
+ info = response[MSG_ARG0];
+
+ return sprintf(buf, "%u.%u.%u.%u\n",
+ (info >> 24) & 0xff,
+ (info >> 16) & 0xff,
+ (info >> 8) & 0xff,
+ info & 0xff);
+}
+
+static ssize_t show_refresh(struct device *dev,
+ struct device_attribute *devattr, char *buf)
+{
+ u32 response[MSG_FIELD_MAX];
+ void __iomem *info;
+ struct private_data *priv;
+ unsigned int offset;
+ u8 refresh, sr_abort, ppre, thermal_offs, tuf;
+ u32 mr4;
+ int ret;
+
+ ret = generic_show(DPFE_CMD_GET_REFRESH, response, dev, buf);
+ if (ret)
+ return ret;
+
+ priv = dev_get_drvdata(dev);
+ offset = response[MSG_ARG0];
+ info = priv->dmem + offset;
+
+ mr4 = dpfe_readl(info + DRAM_INFO_MR4) & DRAM_INFO_MASK;
+
+ refresh = (mr4 >> DRAM_MR4_REFRESH) & DRAM_MR4_REFRESH_MASK;
+ sr_abort = (mr4 >> DRAM_MR4_SR_ABORT) & DRAM_MR4_SR_ABORT_MASK;
+ ppre = (mr4 >> DRAM_MR4_PPRE) & DRAM_MR4_PPRE_MASK;
+ thermal_offs = (mr4 >> DRAM_MR4_TH_OFFS) & DRAM_MR4_TH_OFFS_MASK;
+ tuf = (mr4 >> DRAM_MR4_TUF) & DRAM_MR4_TUF_MASK;
+
+ return sprintf(buf, "%#x %#x %#x %#x %#x %#x %#x\n",
+ dpfe_readl(info + DRAM_INFO_INTERVAL),
+ refresh, sr_abort, ppre, thermal_offs, tuf,
+ dpfe_readl(info + DRAM_INFO_ERROR));
+}
+
+static ssize_t store_refresh(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ u32 response[MSG_FIELD_MAX];
+ struct private_data *priv;
+ void __iomem *info;
+ unsigned int offset;
+ unsigned long val;
+ int ret;
+
+ if (kstrtoul(buf, 0, &val) < 0)
+ return -EINVAL;
+
+ priv = dev_get_drvdata(dev);
+
+ ret = __send_command(priv, DPFE_CMD_GET_REFRESH, response);
+ if (ret)
+ return ret;
+
+ offset = response[MSG_ARG0];
+ info = priv->dmem + offset + DRAM_MR4_REFRESH;
+ dpfe_writel(val, info);
+
+ return count;
+}
+
+
+static ssize_t show_vendor(struct device *dev, struct device_attribute *devattr,
+ char *buf)
+{
+ u32 response[MSG_FIELD_MAX];
+ struct private_data *priv;
+ void __iomem *info;
+ unsigned int offset;
+ int ret;
+
+ ret = generic_show(DPFE_CMD_GET_VENDOR, response, dev, buf);
+ if (ret)
+ return ret;
+
+ offset = response[MSG_ARG0];
+ priv = dev_get_drvdata(dev);
+ info = priv->dmem + offset;
+
+ return sprintf(buf, "%#x %#x %#x %#x %#x\n",
+ dpfe_readl(info + DRAM_VENDOR_MR5) & DRAM_VENDOR_MASK,
+ dpfe_readl(info + DRAM_VENDOR_MR6) & DRAM_VENDOR_MASK,
+ dpfe_readl(info + DRAM_VENDOR_MR7) & DRAM_VENDOR_MASK,
+ dpfe_readl(info + DRAM_VENDOR_MR8) & DRAM_VENDOR_MASK,
+ dpfe_readl(info + DRAM_VENDOR_ERROR));
+}
+
+static SENSOR_DEVICE_ATTR(dpfe_info, 0444, show_info, NULL, 1000);
+static SENSOR_DEVICE_ATTR(dpfe_refresh, 0644, show_refresh, store_refresh,
+ 1000);
+static SENSOR_DEVICE_ATTR(dpfe_vendor, 0444, show_vendor, NULL, 1000);
+static struct attribute *dpfe_attrs[] = {
+ &sensor_dev_attr_dpfe_info.dev_attr.attr,
+ &sensor_dev_attr_dpfe_refresh.dev_attr.attr,
+ &sensor_dev_attr_dpfe_vendor.dev_attr.attr,
+ NULL
+};
+ATTRIBUTE_GROUPS(dpfe);
+
+static int brcmstb_hwmon_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device *hwmon_dev = NULL;
+ struct private_data *priv;
+ struct init_data init;
+ struct resource *res;
+ int ret;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ mutex_init(&priv->lock);
+ platform_set_drvdata(pdev, priv);
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ priv->regs = devm_ioremap_resource(dev, res);
+ if (IS_ERR(priv->regs)) {
+ dev_err(dev, "couldn't map DT entry brcm,dpfe-cpu\n");
+ return -ENODEV;
+ }
+
+ init.dmem = __map_region(DT_COMPAT_DMEM);
+ if (!init.dmem) {
+ dev_err(dev, "Couldn't map %s\n", DT_COMPAT_DMEM);
+ return -ENOENT;
+ }
+ init.imem = __map_region(DT_COMPAT_IMEM);
+ if (init.imem) {
+ ret = brcmstb_hwmon_download_firwmare(pdev, &init);
+ } else {
+ ret = -ENOENT;
+ dev_err(dev, "Couldn't map %s\n", DT_COMPAT_IMEM);
+ }
+
+ /* We don't need IMEM after initialization. */
+ iounmap(init.imem);
+
+ if (!ret) {
+ hwmon_dev = devm_hwmon_device_register_with_groups(dev,
+ "brcmstb_dpfe", priv, dpfe_groups);
+ if (IS_ERR(hwmon_dev))
+ ret = PTR_ERR(hwmon_dev);
+ }
+
+ if (ret) {
+ iounmap(init.dmem);
+ dev_err(dev, "failed to initialize -- error %d\n", ret);
+ } else {
+ priv->dmem = init.dmem;
+ dev_info(dev, "registered.\n");
+ }
+
+ return ret;
+}
+
+static int brcmstb_hwmon_remove(struct platform_device *pdev)
+{
+ struct private_data *priv;
+
+ priv = platform_get_drvdata(pdev);
+ iounmap(priv->dmem);
+
+ return 0;
+}
+
+static const struct of_device_id brcmstb_hwmon_of_match[] = {
+ { .compatible = "brcm,dpfe-cpu", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, brcmstb_hwmon_of_match);
+
+static struct platform_driver brcmstb_hwmon_driver = {
+ .driver = {
+ .name = DRVNAME,
+ .of_match_table = brcmstb_hwmon_of_match,
+ },
+ .probe = brcmstb_hwmon_probe,
+ .remove = brcmstb_hwmon_remove,
+};
+
+module_platform_driver(brcmstb_hwmon_driver);
+
+MODULE_AUTHOR("Markus Mayer <mmayer@broadcom.com>");
+MODULE_DESCRIPTION("BRCMSTB Hardware Monitoring");
+MODULE_LICENSE("GPL");
--
2.7.4
^ permalink raw reply related
* [PATCH 1/2] dt/bindings: Add bindings for Broadcom STB DRAM Sensors
From: Markus Mayer @ 2017-04-18 20:17 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, Rob Herring, Mark Rutland,
Florian Fainelli
Cc: Markus Mayer, Broadcom Kernel List, Linux HWMON List,
Device Tree List, ARM Kernel List, Linux Kernel Mailing List
In-Reply-To: <20170418201702.57019-1-code@mmayer.net>
From: Markus Mayer <mmayer@broadcom.com>
Provide bindings for the Broadcom STB DDR PHY Front End (DPFE).
Signed-off-by: Markus Mayer <mmayer@broadcom.com>
---
.../devicetree/bindings/hwmon/brcmstb-dpfe.txt | 68 ++++++++++++++++++++++
1 file changed, 68 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/brcmstb-dpfe.txt
diff --git a/Documentation/devicetree/bindings/hwmon/brcmstb-dpfe.txt b/Documentation/devicetree/bindings/hwmon/brcmstb-dpfe.txt
new file mode 100644
index 0000000..3519197
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/brcmstb-dpfe.txt
@@ -0,0 +1,68 @@
+DDR PHY Front End (DPFE) for Broadcom STB
+=========================================
+
+DPFE and the DPFE firmware provide an interface for the host CPU to
+communicate with the DCPU, which resides inside the DDR PHY.
+
+There are three memory regions for interacting with the DCPU.
+
+The DCPU Register Space
+-----------------------
+
+Required properties:
+ - compatible: must be one of brcm,bcm7271-dpfe-cpu, brcm,dpfe-cpu-v12.0.0.0
+ or brcm,dpfe-cpu
+ - reg: must reference the start address and length of the DCPU register
+ space
+
+Optional properties:
+ - cell-index: the index of the DPFE instance; will default to 0 if not set
+
+Example:
+ dpfe_cpu0: dpfe-cpu@f1132000 {
+ compatible = "brcm,bcm7271-dpfe-cpu",
+ "brcm,dpfe-cpu-v12.0.0.0",
+ "brcm,dpfe-cpu";
+ reg = <0xf1132000 0x180>;
+ cell-index = <0>;
+ };
+
+The DCPU Data Memory Space
+--------------------------
+
+Required properties:
+ - compatible: must be one of brcm,bcm7271-dpfe-dmem, brcm,dpfe-dmem-v12.0.0.0
+ or brcm,dpfe-dmem
+ - reg: must reference the start address and length of the DCPU DMEM space
+
+Optional properties:
+ - cell-index: the index of the DPFE instance; will default to 0 if not set
+
+Example:
+ dpfe_dmem0: dpfe-dmem@f1134000 {
+ compatible = "brcm,bcm7271-dpfe-dmem",
+ "brcm,dpfe-dmem-v12.0.0.0",
+ "brcm,dpfe-dmem";
+ reg = <0xf1134000 0x1000>;
+ cell-index = <0>;
+ };
+
+The DCPU Instruction Memory Space
+---------------------------------
+
+Required properties:
+ - compatible: must be one of brcm,bcm7271-dpfe-imem, brcm,dpfe-imem-v12.0.0.0
+ or brcm,dpfe-imem
+ - reg: must reference the start address and length of the DCPU IMEM space
+
+Optional properties:
+ - cell-index: the index of the DPFE instance; will default to 0 if not set
+
+Example:
+ dpfe_imem0: dpfe-imem@f1138000 {
+ compatible = "brcm,bcm7271-dpfe-imem",
+ "brcm,dpfe-imem-v12.0.0.0",
+ "brcm,dpfe-imem";
+ reg = <0xf1138000 0x4000>;
+ cell-index = <0>;
+ };
--
2.7.4
^ permalink raw reply related
* [PATCH 0/2] HWMON driver for Broadcom STB DPFE
From: Markus Mayer @ 2017-04-18 20:17 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, Rob Herring, Mark Rutland,
Florian Fainelli
Cc: Markus Mayer, Broadcom Kernel List, Linux HWMON List,
Device Tree List, ARM Kernel List, Linux Kernel Mailing List
From: Markus Mayer <mmayer-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
This series introduces a driver to interact with the Broadcom STB DDR
PHY Front End (DPFE), specifically to communicate with the DCPU that is
part of the DDR PHY and which is running its own firmware.
The DCPU provides information such as DRAM refresh rate, which can be
used as indirect indicator for the DRAM temperature (the higher the
refresh rate, the hotter the RAM).
Markus Mayer (2):
dt/bindings: Add bindings for Broadcom STB DRAM Sensors
hwmon: (brcmstb) Add driver for Broadcom STB DPFE
.../devicetree/bindings/hwmon/brcmstb-dpfe.txt | 68 ++
MAINTAINERS | 8 +
drivers/hwmon/Kconfig | 13 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/brcmstb-dpfe.c | 689 +++++++++++++++++++++
5 files changed, 779 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/brcmstb-dpfe.txt
create mode 100644 drivers/hwmon/brcmstb-dpfe.c
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] arm64: dts: allwinner: a64: Add UART2 pin nodes
From: Andreas Färber @ 2017-04-18 19:32 UTC (permalink / raw)
To: Maxime Ripard
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Chen-Yu Tsai,
Rob Herring, Mark Rutland, Catalin Marinas, Will Deacon,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170418053458.phcbp4da3vatejpg@lukather>
Am 18.04.2017 um 07:34 schrieb Maxime Ripard:
> On Fri, Apr 14, 2017 at 07:13:20PM +0200, Andreas Färber wrote:
>> UART2 is exposed on the Pi connector of Pine64. Make a pinctrl node
>> available at the SoC level, to simplify enabling UART2 via DT overlay.
>>
>> Signed-off-by: Andreas Färber <afaerber-l3A5Bk7waGM@public.gmane.org>
>
> Applied, thanks!
Thanks, that was fast.
> Could you also create the node in the pine64 DTS (with an explicit
> status = "disabled" in there) so that the overlay would be even easier
> to make ?
Makes sense. I went ahead and did it for all UART nodes then - careful,
final patch is neither build- nor runtime-tested yet.
Cheers,
Andreas
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3] arm64: dts: rk3399: add support for firefly-rk3399 board
From: Marc Zyngier @ 2017-04-18 19:31 UTC (permalink / raw)
To: Kever Yang
Cc: heiko-4mtYJXux2i+zQB+pC5nmwQ, Mark Rutland,
devicetree-u79uwXL29TY76Z2rM5mHXA, Catalin Marinas, Brian Norris,
Will Deacon, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Rob Herring,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Matthias Brugger, Andy Yan, Jianqun Xu, Liang Chen
In-Reply-To: <1491796213-4115-1-git-send-email-kever.yang-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
On Mon, 10 Apr 2017 11:50:13 +0800
Kever Yang <kever.yang-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
> Firefly-rk3399 is a bord from T-Firefly, you can find detail about
> it here:
> http://en.t-firefly.com/en/firenow/Firefly_RK3399/
>
> This patch add basic node for the board and make it able to bring
> up.
>
> Peripheral works:
> - usb hub which connect to ehci controller;
> - UART2 debug
> - eMMC
> - PCIe
>
> Not work:
> - USB 3.0 HOST, type-C port
> - sdio, sd-card
>
> Not test for other peripheral:
[...]
> - Ethernet
[...]
FWIW, I've tested the Ethernet port, which seems to work nicely.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH] arm64: dts: allwinner: pine64: Prepare optional UART nodes with pinctrl
From: Andreas Färber @ 2017-04-18 19:25 UTC (permalink / raw)
To: Maxime Ripard
Cc: Andreas Färber, Chen-Yu Tsai, Rob Herring, Mark Rutland,
Catalin Marinas, Will Deacon, linux-arm-kernel, devicetree,
linux-kernel
In-Reply-To: <20170418053458.phcbp4da3vatejpg@lukather>
Pine64 exposes all A64 UARTs, not just UART0.
Since the pins can be used as GPIO, don't enable the new UART nodes by
default, but prepare the pinctrl settings to aid in activating them via
overlays, i.e., overriding the status property of &uartX nodes.
For UART4 (Euler) the safer route of not including RTS/CTS pins is chosen,
whereas for UART1 (Bluetooth) they are included.
Add the corresponding pinctrl nodes where missing.
Suggested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
.../arm64/boot/dts/allwinner/sun50i-a64-pine64.dts | 29 ++++++++++++++++++++++
arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 15 +++++++++++
2 files changed, 44 insertions(+)
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
index db6c0f36999e..260fb12ac3fc 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
@@ -99,12 +99,41 @@
status = "okay";
};
+/* On Exp and Euler connectors */
&uart0 {
pinctrl-names = "default";
pinctrl-0 = <&uart0_pins_a>;
status = "okay";
};
+/* On Wifi/BT connector, with RTS/CTS */
+&uart1 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&uart1_pins>, <&uart1_rts_cts_pins>;
+ status = "disabled";
+};
+
+/* On Pi-2 connector */
+&uart2 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&uart2_pins>;
+ status = "disabled";
+};
+
+/* On Euler connector */
+&uart3 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&uart3_pins>;
+ status = "disabled";
+};
+
+/* On Euler connector, RTS/CTS optional */
+&uart4 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&uart4_pins>;
+ status = "disabled";
+};
+
&usb_otg {
dr_mode = "host";
status = "okay";
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index 133810df06b9..c172c62447e8 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -292,6 +292,21 @@
pins = "PB0", "PB1";
function = "uart2";
};
+
+ uart3_pins: uart3-pins {
+ pins = "PD0", "PD1";
+ function = "uart3";
+ };
+
+ uart4_pins: uart4-pins {
+ pins = "PD2", "PD3";
+ function = "uart4";
+ };
+
+ uart4_rts_cts_pins: uart4-rts-cts-pins {
+ pins = "PD4", "PD5";
+ function = "uart4";
+ };
};
uart0: serial@1c28000 {
--
2.10.2
^ permalink raw reply related
* Re: [PATCH 1/3] drm/vc4: Turn the V3D clock on at runtime.
From: Eric Anholt @ 2017-04-18 19:23 UTC (permalink / raw)
To: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Rob Herring,
Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170418191157.18517-1-eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 720 bytes --]
Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org> writes:
> For the Raspberry Pi's bindings, the power domain also implicitly
> turns on the clock and deasserts reset, but for the new Cygnus port we
> start representing the clock in the devicetree.
> + v3d->clk = devm_clk_get(dev, "v3d_clk");
> + if (IS_ERR(v3d->clk)) {
> + int ret = PTR_ERR(v3d->clk);
> +
> + if (ret == -ENODEV) {
Apparently I hadn't booted this on RPi yet. This is supposed to be
-ENOENT.
> + /* bcm2835 didn't have a clock reference in the DT. */
> + ret = 0;
> + v3d->clk = NULL;
> + } else {
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "Failed to get V3D clock: %d\n",
> + ret);
> + return ret;
> + }
> + }
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* [PATCH 3/4] of/fdt: introduce of_scan_flat_dt_subnodes and of_get_flat_dt_phandle
From: Nicholas Piggin @ 2017-04-18 19:12 UTC (permalink / raw)
To: linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ
Cc: Nicholas Piggin, devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170418191220.3166-1-npiggin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Introduce primitives for FDT parsing. These will be used for powerpc
cpufeatures node scanning, which has quite complex structure but should
be processed early.
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Signed-off-by: Nicholas Piggin <npiggin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
drivers/of/fdt.c | 38 ++++++++++++++++++++++++++++++++++++++
include/linux/of_fdt.h | 6 ++++++
2 files changed, 44 insertions(+)
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index e5ce4b59e162..961ca97072a9 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -754,6 +754,36 @@ int __init of_scan_flat_dt(int (*it)(unsigned long node,
}
/**
+ * of_scan_flat_dt_subnodes - scan sub-nodes of a node call callback on each.
+ * @it: callback function
+ * @data: context data pointer
+ *
+ * This function is used to scan sub-nodes of a node.
+ */
+int __init of_scan_flat_dt_subnodes(unsigned long parent,
+ int (*it)(unsigned long node,
+ const char *uname,
+ void *data),
+ void *data)
+{
+ const void *blob = initial_boot_params;
+ int node;
+
+ fdt_for_each_subnode(node, blob, parent) {
+ const char *pathp;
+ int rc;
+
+ pathp = fdt_get_name(blob, node, NULL);
+ if (*pathp == '/')
+ pathp = kbasename(pathp);
+ rc = it(node, pathp, data);
+ if (rc)
+ return rc;
+ }
+ return 0;
+}
+
+/**
* of_get_flat_dt_subnode_by_name - get the subnode by given name
*
* @node: the parent node
@@ -812,6 +842,14 @@ int __init of_flat_dt_match(unsigned long node, const char *const *compat)
return of_fdt_match(initial_boot_params, node, compat);
}
+/**
+ * of_get_flat_dt_prop - Given a node in the flat blob, return the phandle
+ */
+uint32_t __init of_get_flat_dt_phandle(unsigned long node)
+{
+ return fdt_get_phandle(initial_boot_params, node);
+}
+
struct fdt_scan_status {
const char *name;
int namelen;
diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
index 271b3fdf0070..1dfbfd0d8040 100644
--- a/include/linux/of_fdt.h
+++ b/include/linux/of_fdt.h
@@ -54,6 +54,11 @@ extern char __dtb_end[];
extern int of_scan_flat_dt(int (*it)(unsigned long node, const char *uname,
int depth, void *data),
void *data);
+extern int of_scan_flat_dt_subnodes(unsigned long node,
+ int (*it)(unsigned long node,
+ const char *uname,
+ void *data),
+ void *data);
extern int of_get_flat_dt_subnode_by_name(unsigned long node,
const char *uname);
extern const void *of_get_flat_dt_prop(unsigned long node, const char *name,
@@ -62,6 +67,7 @@ extern int of_flat_dt_is_compatible(unsigned long node, const char *name);
extern int of_flat_dt_match(unsigned long node, const char *const *matches);
extern unsigned long of_get_flat_dt_root(void);
extern int of_get_flat_dt_size(void);
+extern uint32_t of_get_flat_dt_phandle(unsigned long node);
extern int early_init_dt_scan_chosen(unsigned long node, const char *uname,
int depth, void *data);
--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH 3/3] drm/vc4: Add specific compatible strings for Cygnus.
From: Eric Anholt @ 2017-04-18 19:11 UTC (permalink / raw)
To: dri-devel, Rob Herring, Mark Rutland, devicetree; +Cc: linux-kernel
In-Reply-To: <20170418191157.18517-1-eric@anholt.net>
Cygnus has V3D 2.6 instead of 2.1, and doesn't use the VC4 display
modules. The V3D can be uniquely identified by the IDENT[01]
registers, and there's nothing to key off of for the display change
other than the lack of DT nodes for the display components, but it's
convention to have new compatible strings anyway.
Signed-off-by: Eric Anholt <eric@anholt.net>
---
Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt | 4 ++--
drivers/gpu/drm/vc4/vc4_drv.c | 1 +
drivers/gpu/drm/vc4/vc4_v3d.c | 1 +
3 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt b/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt
index bc1756f4f791..284e2b14cfbe 100644
--- a/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt
+++ b/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt
@@ -5,7 +5,7 @@ with HDMI output and the HVS (Hardware Video Scaler) for compositing
display planes.
Required properties for VC4:
-- compatible: Should be "brcm,bcm2835-vc4"
+- compatible: Should be "brcm,bcm2835-vc4" or "brcm,cygnus-vc4"
Required properties for Pixel Valve:
- compatible: Should be one of "brcm,bcm2835-pixelvalve0",
@@ -54,7 +54,7 @@ Required properties for VEC:
See bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt
Required properties for V3D:
-- compatible: Should be "brcm,bcm2835-v3d"
+- compatible: Should be "brcm,bcm2835-v3d" or "brcm,cygnus-v3d"
- reg: Physical base address and length of the V3D's registers
- interrupts: The interrupt number
See bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt
diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index 92fb9a41fe7c..754ce76d4b98 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -335,6 +335,7 @@ static int vc4_platform_drm_remove(struct platform_device *pdev)
static const struct of_device_id vc4_of_match[] = {
{ .compatible = "brcm,bcm2835-vc4", },
+ { .compatible = "brcm,cygnus-vc4", },
{},
};
MODULE_DEVICE_TABLE(of, vc4_of_match);
diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
index 2442622e6bff..3abcd27aa46f 100644
--- a/drivers/gpu/drm/vc4/vc4_v3d.c
+++ b/drivers/gpu/drm/vc4/vc4_v3d.c
@@ -304,6 +304,7 @@ static int vc4_v3d_dev_remove(struct platform_device *pdev)
static const struct of_device_id vc4_v3d_dt_match[] = {
{ .compatible = "brcm,bcm2835-v3d" },
+ { .compatible = "brcm,cygnus-v3d" },
{ .compatible = "brcm,vc4-v3d" },
{}
};
--
2.11.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
* [PATCH 2/3] drm/vc4: Don't try to initialize FBDEV if we're only bound to V3D.
From: Eric Anholt @ 2017-04-18 19:11 UTC (permalink / raw)
To: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Rob Herring,
Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, Eric Anholt
In-Reply-To: <20170418191157.18517-1-eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
The FBDEV initialization would throw an error in dmesg, when we just
want to silently not initialize fbdev on a V3D-only VC4 instance.
Signed-off-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
---
drivers/gpu/drm/vc4/vc4_kms.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index ad7925a9e0ea..237a504f11f0 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -230,10 +230,12 @@ int vc4_kms_load(struct drm_device *dev)
drm_mode_config_reset(dev);
- vc4->fbdev = drm_fbdev_cma_init(dev, 32,
- dev->mode_config.num_connector);
- if (IS_ERR(vc4->fbdev))
- vc4->fbdev = NULL;
+ if (dev->mode_config.num_connector) {
+ vc4->fbdev = drm_fbdev_cma_init(dev, 32,
+ dev->mode_config.num_connector);
+ if (IS_ERR(vc4->fbdev))
+ vc4->fbdev = NULL;
+ }
drm_kms_helper_poll_init(dev);
--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH 1/3] drm/vc4: Turn the V3D clock on at runtime.
From: Eric Anholt @ 2017-04-18 19:11 UTC (permalink / raw)
To: dri-devel, Rob Herring, Mark Rutland, devicetree; +Cc: linux-kernel
For the Raspberry Pi's bindings, the power domain also implicitly
turns on the clock and deasserts reset, but for the new Cygnus port we
start representing the clock in the devicetree.
Signed-off-by: Eric Anholt <eric@anholt.net>
---
.../devicetree/bindings/display/brcm,bcm-vc4.txt | 3 ++
drivers/gpu/drm/vc4/vc4_drv.h | 1 +
drivers/gpu/drm/vc4/vc4_v3d.c | 33 ++++++++++++++++++++++
3 files changed, 37 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt b/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt
index ca02d3e4db91..bc1756f4f791 100644
--- a/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt
+++ b/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt
@@ -59,6 +59,9 @@ Required properties for V3D:
- interrupts: The interrupt number
See bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt
+Optional properties for V3D:
+- clocks: The clock the unit runs on
+
Required properties for DSI:
- compatible: Should be "brcm,bcm2835-dsi0" or "brcm,bcm2835-dsi1"
- reg: Physical base address and length of the DSI block's registers
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 81d2bc08e766..08d5c2213c80 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -189,6 +189,7 @@ struct vc4_v3d {
struct vc4_dev *vc4;
struct platform_device *pdev;
void __iomem *regs;
+ struct clk *clk;
};
struct vc4_hvs {
diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
index 7cc346ad9b0b..2442622e6bff 100644
--- a/drivers/gpu/drm/vc4/vc4_v3d.c
+++ b/drivers/gpu/drm/vc4/vc4_v3d.c
@@ -16,6 +16,7 @@
* this program. If not, see <http://www.gnu.org/licenses/>.
*/
+#include "linux/clk.h"
#include "linux/component.h"
#include "linux/pm_runtime.h"
#include "vc4_drv.h"
@@ -164,6 +165,9 @@ static int vc4_v3d_runtime_suspend(struct device *dev)
vc4_irq_uninstall(vc4->dev);
+ if (v3d->clk)
+ clk_disable_unprepare(v3d->clk);
+
return 0;
}
@@ -172,6 +176,13 @@ static int vc4_v3d_runtime_resume(struct device *dev)
struct vc4_v3d *v3d = dev_get_drvdata(dev);
struct vc4_dev *vc4 = v3d->vc4;
+ if (v3d->clk) {
+ int ret = clk_prepare_enable(v3d->clk);
+
+ if (ret != 0)
+ return ret;
+ }
+
vc4_v3d_init_hw(vc4->dev);
vc4_irq_postinstall(vc4->dev);
@@ -202,12 +213,34 @@ static int vc4_v3d_bind(struct device *dev, struct device *master, void *data)
vc4->v3d = v3d;
v3d->vc4 = vc4;
+ v3d->clk = devm_clk_get(dev, "v3d_clk");
+ if (IS_ERR(v3d->clk)) {
+ int ret = PTR_ERR(v3d->clk);
+
+ if (ret == -ENODEV) {
+ /* bcm2835 didn't have a clock reference in the DT. */
+ ret = 0;
+ v3d->clk = NULL;
+ } else {
+ if (ret != -EPROBE_DEFER)
+ dev_err(dev, "Failed to get V3D clock: %d\n",
+ ret);
+ return ret;
+ }
+ }
+
if (V3D_READ(V3D_IDENT0) != V3D_EXPECTED_IDENT0) {
DRM_ERROR("V3D_IDENT0 read 0x%08x instead of 0x%08x\n",
V3D_READ(V3D_IDENT0), V3D_EXPECTED_IDENT0);
return -EINVAL;
}
+ if (v3d->clk) {
+ ret = clk_prepare_enable(v3d->clk);
+ if (ret != 0)
+ return ret;
+ }
+
/* Reset the binner overflow address/size at setup, to be sure
* we don't reuse an old one.
*/
--
2.11.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
* [PATCH 2/2] arm64: dts: juno: add information about L1 and L2 caches
From: Sudeep Holla @ 2017-04-18 17:57 UTC (permalink / raw)
To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: Sudeep Holla, Will Deacon, Liviu Dudau
In-Reply-To: <1492538279-16405-1-git-send-email-sudeep.holla-5wv7dgnIgG8@public.gmane.org>
Commit a8d4636f96ad ("arm64: cacheinfo: Remove CCSIDR-based cache
information probing") removed mechanism to extract cache information
based on CCSIDR register as the architecture explicitly states no
inference about the actual sizes of caches based on CCSIDR registers.
Commit 9a802431c527 ("arm64: cacheinfo: add support to override cache
levels via device tree") had already provided options to override cache
information from the device tree.
This patch adds the information about L1 and L2 caches on all variants
of Juno platform.
Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
Cc: Liviu Dudau <liviu.dudau-5wv7dgnIgG8@public.gmane.org>
Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
---
arch/arm64/boot/dts/arm/juno-r1.dts | 42 +++++++++++++++++++++++++++++++++++++
arch/arm64/boot/dts/arm/juno-r2.dts | 42 +++++++++++++++++++++++++++++++++++++
arch/arm64/boot/dts/arm/juno.dts | 42 +++++++++++++++++++++++++++++++++++++
3 files changed, 126 insertions(+)
diff --git a/arch/arm64/boot/dts/arm/juno-r1.dts b/arch/arm64/boot/dts/arm/juno-r1.dts
index 0033c59a64b5..0e8943ab94d7 100644
--- a/arch/arm64/boot/dts/arm/juno-r1.dts
+++ b/arch/arm64/boot/dts/arm/juno-r1.dts
@@ -89,6 +89,12 @@
reg = <0x0 0x0>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0xc000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <256>;
next-level-cache = <&A57_L2>;
clocks = <&scpi_dvfs 0>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -100,6 +106,12 @@
reg = <0x0 0x1>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0xc000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <256>;
next-level-cache = <&A57_L2>;
clocks = <&scpi_dvfs 0>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -111,6 +123,12 @@
reg = <0x0 0x100>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -122,6 +140,12 @@
reg = <0x0 0x101>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -133,6 +157,12 @@
reg = <0x0 0x102>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -144,6 +174,12 @@
reg = <0x0 0x103>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -152,10 +188,16 @@
A57_L2: l2-cache0 {
compatible = "cache";
+ cache-size = <0x200000>;
+ cache-line-size = <64>;
+ cache-sets = <2048>;
};
A53_L2: l2-cache1 {
compatible = "cache";
+ cache-size = <0x100000>;
+ cache-line-size = <64>;
+ cache-sets = <1024>;
};
};
diff --git a/arch/arm64/boot/dts/arm/juno-r2.dts b/arch/arm64/boot/dts/arm/juno-r2.dts
index 218d0e4736a8..405e2fba025b 100644
--- a/arch/arm64/boot/dts/arm/juno-r2.dts
+++ b/arch/arm64/boot/dts/arm/juno-r2.dts
@@ -89,6 +89,12 @@
reg = <0x0 0x0>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0xc000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <256>;
next-level-cache = <&A72_L2>;
clocks = <&scpi_dvfs 0>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -100,6 +106,12 @@
reg = <0x0 0x1>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0xc000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <256>;
next-level-cache = <&A72_L2>;
clocks = <&scpi_dvfs 0>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -111,6 +123,12 @@
reg = <0x0 0x100>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -122,6 +140,12 @@
reg = <0x0 0x101>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -133,6 +157,12 @@
reg = <0x0 0x102>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -144,6 +174,12 @@
reg = <0x0 0x103>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -152,10 +188,16 @@
A72_L2: l2-cache0 {
compatible = "cache";
+ cache-size = <0x200000>;
+ cache-line-size = <64>;
+ cache-sets = <2048>;
};
A53_L2: l2-cache1 {
compatible = "cache";
+ cache-size = <0x100000>;
+ cache-line-size = <64>;
+ cache-sets = <1024>;
};
};
diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
index bb2820ef3d5b..0220494c9b80 100644
--- a/arch/arm64/boot/dts/arm/juno.dts
+++ b/arch/arm64/boot/dts/arm/juno.dts
@@ -88,6 +88,12 @@
reg = <0x0 0x0>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0xc000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <256>;
next-level-cache = <&A57_L2>;
clocks = <&scpi_dvfs 0>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -99,6 +105,12 @@
reg = <0x0 0x1>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0xc000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <256>;
next-level-cache = <&A57_L2>;
clocks = <&scpi_dvfs 0>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -110,6 +122,12 @@
reg = <0x0 0x100>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -121,6 +139,12 @@
reg = <0x0 0x101>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -132,6 +156,12 @@
reg = <0x0 0x102>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -143,6 +173,12 @@
reg = <0x0 0x103>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -151,10 +187,16 @@
A57_L2: l2-cache0 {
compatible = "cache";
+ cache-size = <0x200000>;
+ cache-line-size = <64>;
+ cache-sets = <2048>;
};
A53_L2: l2-cache1 {
compatible = "cache";
+ cache-size = <0x100000>;
+ cache-line-size = <64>;
+ cache-sets = <1024>;
};
};
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH 1/2] arm64: dts: juno: fix few unit address format warnings
From: Sudeep Holla @ 2017-04-18 17:57 UTC (permalink / raw)
To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: Sudeep Holla, Liviu Dudau
This patch fixes the following set of warnings on juno.
smb@08000000 unit name should not have leading 0s
sysctl@020000 simple-bus unit address format error, expected "20000"
apbregs@010000 simple-bus unit address format error, expected "10000"
mmci@050000 simple-bus unit address format error, expected "50000"
kmi@060000 simple-bus unit address format error, expected "60000"
kmi@070000 simple-bus unit address format error, expected "70000"
wdt@0f0000 simple-bus unit address format error, expected "f0000"
Cc: Liviu Dudau <liviu.dudau-5wv7dgnIgG8@public.gmane.org>
Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
---
arch/arm64/boot/dts/arm/juno-base.dtsi | 2 +-
arch/arm64/boot/dts/arm/juno-motherboard.dtsi | 12 ++++++------
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/arch/arm64/boot/dts/arm/juno-base.dtsi b/arch/arm64/boot/dts/arm/juno-base.dtsi
index 8ffaff2043d0..bfe7d683a42e 100644
--- a/arch/arm64/boot/dts/arm/juno-base.dtsi
+++ b/arch/arm64/boot/dts/arm/juno-base.dtsi
@@ -699,7 +699,7 @@
<0x00000008 0x80000000 0x1 0x80000000>;
};
- smb@08000000 {
+ smb@8000000 {
compatible = "simple-bus";
#address-cells = <2>;
#size-cells = <1>;
diff --git a/arch/arm64/boot/dts/arm/juno-motherboard.dtsi b/arch/arm64/boot/dts/arm/juno-motherboard.dtsi
index 098601657f82..2ac43221ddb6 100644
--- a/arch/arm64/boot/dts/arm/juno-motherboard.dtsi
+++ b/arch/arm64/boot/dts/arm/juno-motherboard.dtsi
@@ -137,7 +137,7 @@
#size-cells = <1>;
ranges = <0 3 0 0x200000>;
- v2m_sysctl: sysctl@020000 {
+ v2m_sysctl: sysctl@20000 {
compatible = "arm,sp810", "arm,primecell";
reg = <0x020000 0x1000>;
clocks = <&v2m_refclk32khz>, <&v2m_refclk1mhz>, <&mb_clk24mhz>;
@@ -148,7 +148,7 @@
assigned-clock-parents = <&v2m_refclk1mhz>, <&v2m_refclk1mhz>, <&v2m_refclk1mhz>, <&v2m_refclk1mhz>;
};
- apbregs@010000 {
+ apbregs@10000 {
compatible = "syscon", "simple-mfd";
reg = <0x010000 0x1000>;
@@ -216,7 +216,7 @@
};
};
- mmci@050000 {
+ mmci@50000 {
compatible = "arm,pl180", "arm,primecell";
reg = <0x050000 0x1000>;
interrupts = <5>;
@@ -228,7 +228,7 @@
clock-names = "mclk", "apb_pclk";
};
- kmi@060000 {
+ kmi@60000 {
compatible = "arm,pl050", "arm,primecell";
reg = <0x060000 0x1000>;
interrupts = <8>;
@@ -236,7 +236,7 @@
clock-names = "KMIREFCLK", "apb_pclk";
};
- kmi@070000 {
+ kmi@70000 {
compatible = "arm,pl050", "arm,primecell";
reg = <0x070000 0x1000>;
interrupts = <8>;
@@ -244,7 +244,7 @@
clock-names = "KMIREFCLK", "apb_pclk";
};
- wdt@0f0000 {
+ wdt@f0000 {
compatible = "arm,sp805", "arm,primecell";
reg = <0x0f0000 0x10000>;
interrupts = <7>;
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH] ARM: dts: vexpress: fix few unit address format warnings
From: Sudeep Holla @ 2017-04-18 17:45 UTC (permalink / raw)
To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: Sudeep Holla, Liviu Dudau, Lorenzo Pieralisi
This patch fixes the following set of warnings on vexpress platforms:
sysreg@010000 simple-bus unit address format error, expected "10000"
sysctl@020000 simple-bus unit address format error, expected "20000"
i2c@030000 simple-bus unit address format error, expected "30000"
aaci@040000 simple-bus unit address format error, expected "40000"
mmci@050000 simple-bus unit address format error, expected "50000"
kmi@060000 simple-bus unit address format error, expected "60000"
kmi@070000 simple-bus unit address format error, expected "70000"
uart@090000 simple-bus unit address format error, expected "90000"
uart@0a0000 simple-bus unit address format error, expected "a0000"
uart@0b0000 simple-bus unit address format error, expected "b0000"
uart@0c0000 simple-bus unit address format error, expected "c0000"
wdt@0f0000 simple-bus unit address format error, expected "f0000"
Cc: Liviu Dudau <liviu.dudau-5wv7dgnIgG8@public.gmane.org>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org>
Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
---
arch/arm/boot/dts/vexpress-v2m-rs1.dtsi | 24 ++++++++++++------------
arch/arm/boot/dts/vexpress-v2m.dtsi | 24 ++++++++++++------------
arch/arm/boot/dts/vexpress-v2p-ca15-tc1.dts | 2 +-
arch/arm/boot/dts/vexpress-v2p-ca15_a7.dts | 18 +++++++++---------
arch/arm/boot/dts/vexpress-v2p-ca5s.dts | 2 +-
arch/arm/boot/dts/vexpress-v2p-ca9.dts | 2 +-
6 files changed, 36 insertions(+), 36 deletions(-)
Hi,
I observed few warning in linux-next due to the enhanced DTC checks
introduced with DTC upgrade in linux-next. The patch fixes few warnings
Regards,
Sudeep
diff --git a/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi b/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
index 3086efacd00e..35714ff6f467 100644
--- a/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
+++ b/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
@@ -71,7 +71,7 @@
#size-cells = <1>;
ranges = <0 3 0 0x200000>;
- v2m_sysreg: sysreg@010000 {
+ v2m_sysreg: sysreg@10000 {
compatible = "arm,vexpress-sysreg";
reg = <0x010000 0x1000>;
@@ -94,7 +94,7 @@
};
};
- v2m_sysctl: sysctl@020000 {
+ v2m_sysctl: sysctl@20000 {
compatible = "arm,sp810", "arm,primecell";
reg = <0x020000 0x1000>;
clocks = <&v2m_refclk32khz>, <&v2m_refclk1mhz>, <&smbclk>;
@@ -106,7 +106,7 @@
};
/* PCI-E I2C bus */
- v2m_i2c_pcie: i2c@030000 {
+ v2m_i2c_pcie: i2c@30000 {
compatible = "arm,versatile-i2c";
reg = <0x030000 0x1000>;
@@ -119,7 +119,7 @@
};
};
- aaci@040000 {
+ aaci@40000 {
compatible = "arm,pl041", "arm,primecell";
reg = <0x040000 0x1000>;
interrupts = <11>;
@@ -127,7 +127,7 @@
clock-names = "apb_pclk";
};
- mmci@050000 {
+ mmci@50000 {
compatible = "arm,pl180", "arm,primecell";
reg = <0x050000 0x1000>;
interrupts = <9 10>;
@@ -139,7 +139,7 @@
clock-names = "mclk", "apb_pclk";
};
- kmi@060000 {
+ kmi@60000 {
compatible = "arm,pl050", "arm,primecell";
reg = <0x060000 0x1000>;
interrupts = <12>;
@@ -147,7 +147,7 @@
clock-names = "KMIREFCLK", "apb_pclk";
};
- kmi@070000 {
+ kmi@70000 {
compatible = "arm,pl050", "arm,primecell";
reg = <0x070000 0x1000>;
interrupts = <13>;
@@ -155,7 +155,7 @@
clock-names = "KMIREFCLK", "apb_pclk";
};
- v2m_serial0: uart@090000 {
+ v2m_serial0: uart@90000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x090000 0x1000>;
interrupts = <5>;
@@ -163,7 +163,7 @@
clock-names = "uartclk", "apb_pclk";
};
- v2m_serial1: uart@0a0000 {
+ v2m_serial1: uart@a0000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x0a0000 0x1000>;
interrupts = <6>;
@@ -171,7 +171,7 @@
clock-names = "uartclk", "apb_pclk";
};
- v2m_serial2: uart@0b0000 {
+ v2m_serial2: uart@b0000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x0b0000 0x1000>;
interrupts = <7>;
@@ -179,7 +179,7 @@
clock-names = "uartclk", "apb_pclk";
};
- v2m_serial3: uart@0c0000 {
+ v2m_serial3: uart@c0000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x0c0000 0x1000>;
interrupts = <8>;
@@ -187,7 +187,7 @@
clock-names = "uartclk", "apb_pclk";
};
- wdt@0f0000 {
+ wdt@f0000 {
compatible = "arm,sp805", "arm,primecell";
reg = <0x0f0000 0x1000>;
interrupts = <0>;
diff --git a/arch/arm/boot/dts/vexpress-v2m.dtsi b/arch/arm/boot/dts/vexpress-v2m.dtsi
index c6393d3f1719..1b6f6393be93 100644
--- a/arch/arm/boot/dts/vexpress-v2m.dtsi
+++ b/arch/arm/boot/dts/vexpress-v2m.dtsi
@@ -70,7 +70,7 @@
#size-cells = <1>;
ranges = <0 7 0 0x20000>;
- v2m_sysreg: sysreg@00000 {
+ v2m_sysreg: sysreg@0 {
compatible = "arm,vexpress-sysreg";
reg = <0x00000 0x1000>;
@@ -93,7 +93,7 @@
};
};
- v2m_sysctl: sysctl@01000 {
+ v2m_sysctl: sysctl@1000 {
compatible = "arm,sp810", "arm,primecell";
reg = <0x01000 0x1000>;
clocks = <&v2m_refclk32khz>, <&v2m_refclk1mhz>, <&smbclk>;
@@ -105,7 +105,7 @@
};
/* PCI-E I2C bus */
- v2m_i2c_pcie: i2c@02000 {
+ v2m_i2c_pcie: i2c@2000 {
compatible = "arm,versatile-i2c";
reg = <0x02000 0x1000>;
@@ -118,7 +118,7 @@
};
};
- aaci@04000 {
+ aaci@4000 {
compatible = "arm,pl041", "arm,primecell";
reg = <0x04000 0x1000>;
interrupts = <11>;
@@ -126,7 +126,7 @@
clock-names = "apb_pclk";
};
- mmci@05000 {
+ mmci@5000 {
compatible = "arm,pl180", "arm,primecell";
reg = <0x05000 0x1000>;
interrupts = <9 10>;
@@ -138,7 +138,7 @@
clock-names = "mclk", "apb_pclk";
};
- kmi@06000 {
+ kmi@6000 {
compatible = "arm,pl050", "arm,primecell";
reg = <0x06000 0x1000>;
interrupts = <12>;
@@ -146,7 +146,7 @@
clock-names = "KMIREFCLK", "apb_pclk";
};
- kmi@07000 {
+ kmi@7000 {
compatible = "arm,pl050", "arm,primecell";
reg = <0x07000 0x1000>;
interrupts = <13>;
@@ -154,7 +154,7 @@
clock-names = "KMIREFCLK", "apb_pclk";
};
- v2m_serial0: uart@09000 {
+ v2m_serial0: uart@9000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x09000 0x1000>;
interrupts = <5>;
@@ -162,7 +162,7 @@
clock-names = "uartclk", "apb_pclk";
};
- v2m_serial1: uart@0a000 {
+ v2m_serial1: uart@a000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x0a000 0x1000>;
interrupts = <6>;
@@ -170,7 +170,7 @@
clock-names = "uartclk", "apb_pclk";
};
- v2m_serial2: uart@0b000 {
+ v2m_serial2: uart@b000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x0b000 0x1000>;
interrupts = <7>;
@@ -178,7 +178,7 @@
clock-names = "uartclk", "apb_pclk";
};
- v2m_serial3: uart@0c000 {
+ v2m_serial3: uart@c000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x0c000 0x1000>;
interrupts = <8>;
@@ -186,7 +186,7 @@
clock-names = "uartclk", "apb_pclk";
};
- wdt@0f000 {
+ wdt@f000 {
compatible = "arm,sp805", "arm,primecell";
reg = <0x0f000 0x1000>;
interrupts = <0>;
diff --git a/arch/arm/boot/dts/vexpress-v2p-ca15-tc1.dts b/arch/arm/boot/dts/vexpress-v2p-ca15-tc1.dts
index 15f4fd3f4695..0c8de0ca73ee 100644
--- a/arch/arm/boot/dts/vexpress-v2p-ca15-tc1.dts
+++ b/arch/arm/boot/dts/vexpress-v2p-ca15-tc1.dts
@@ -220,7 +220,7 @@
};
};
- smb@08000000 {
+ smb@8000000 {
compatible = "simple-bus";
#address-cells = <2>;
diff --git a/arch/arm/boot/dts/vexpress-v2p-ca15_a7.dts b/arch/arm/boot/dts/vexpress-v2p-ca15_a7.dts
index bd107c5a0226..65ecf206388c 100644
--- a/arch/arm/boot/dts/vexpress-v2p-ca15_a7.dts
+++ b/arch/arm/boot/dts/vexpress-v2p-ca15_a7.dts
@@ -385,7 +385,7 @@
};
};
- etb@0,20010000 {
+ etb@20010000 {
compatible = "arm,coresight-etb10", "arm,primecell";
reg = <0 0x20010000 0 0x1000>;
@@ -399,7 +399,7 @@
};
};
- tpiu@0,20030000 {
+ tpiu@20030000 {
compatible = "arm,coresight-tpiu", "arm,primecell";
reg = <0 0x20030000 0 0x1000>;
@@ -449,7 +449,7 @@
};
};
- funnel@0,20040000 {
+ funnel@20040000 {
compatible = "arm,coresight-funnel", "arm,primecell";
reg = <0 0x20040000 0 0x1000>;
@@ -513,7 +513,7 @@
};
};
- ptm@0,2201c000 {
+ ptm@2201c000 {
compatible = "arm,coresight-etm3x", "arm,primecell";
reg = <0 0x2201c000 0 0x1000>;
@@ -527,7 +527,7 @@
};
};
- ptm@0,2201d000 {
+ ptm@2201d000 {
compatible = "arm,coresight-etm3x", "arm,primecell";
reg = <0 0x2201d000 0 0x1000>;
@@ -541,7 +541,7 @@
};
};
- etm@0,2203c000 {
+ etm@2203c000 {
compatible = "arm,coresight-etm3x", "arm,primecell";
reg = <0 0x2203c000 0 0x1000>;
@@ -555,7 +555,7 @@
};
};
- etm@0,2203d000 {
+ etm@2203d000 {
compatible = "arm,coresight-etm3x", "arm,primecell";
reg = <0 0x2203d000 0 0x1000>;
@@ -569,7 +569,7 @@
};
};
- etm@0,2203e000 {
+ etm@2203e000 {
compatible = "arm,coresight-etm3x", "arm,primecell";
reg = <0 0x2203e000 0 0x1000>;
@@ -583,7 +583,7 @@
};
};
- smb@08000000 {
+ smb@8000000 {
compatible = "simple-bus";
#address-cells = <2>;
diff --git a/arch/arm/boot/dts/vexpress-v2p-ca5s.dts b/arch/arm/boot/dts/vexpress-v2p-ca5s.dts
index 1acecaf4b13d..6e69b8e6c1a7 100644
--- a/arch/arm/boot/dts/vexpress-v2p-ca5s.dts
+++ b/arch/arm/boot/dts/vexpress-v2p-ca5s.dts
@@ -190,7 +190,7 @@
};
};
- smb@08000000 {
+ smb@8000000 {
compatible = "simple-bus";
#address-cells = <2>;
diff --git a/arch/arm/boot/dts/vexpress-v2p-ca9.dts b/arch/arm/boot/dts/vexpress-v2p-ca9.dts
index b608a03ee02f..c9305b58afc2 100644
--- a/arch/arm/boot/dts/vexpress-v2p-ca9.dts
+++ b/arch/arm/boot/dts/vexpress-v2p-ca9.dts
@@ -300,7 +300,7 @@
};
};
- smb@04000000 {
+ smb@4000000 {
compatible = "simple-bus";
#address-cells = <2>;
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH v6 6/8] coresight: add support for CPU debug module
From: Mathieu Poirier @ 2017-04-18 17:40 UTC (permalink / raw)
To: Leo Yan
Cc: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
Catalin Marinas, Will Deacon, Andy Gross, David Brown,
Suzuki K Poulose, Stephen Boyd, linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
linux-soc-u79uwXL29TY76Z2rM5mHXA, Mike Leach, Sudeep Holla
In-Reply-To: <1491485461-22800-7-git-send-email-leo.yan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
On Thu, Apr 06, 2017 at 09:30:59PM +0800, Leo Yan wrote:
> Coresight includes debug module and usually the module connects with CPU
> debug logic. ARMv8 architecture reference manual (ARM DDI 0487A.k) has
> description for related info in "Part H: External Debug".
>
> Chapter H7 "The Sample-based Profiling Extension" introduces several
> sampling registers, e.g. we can check program counter value with
> combined CPU exception level, secure state, etc. So this is helpful for
> analysis CPU lockup scenarios, e.g. if one CPU has run into infinite
> loop with IRQ disabled. In this case the CPU cannot switch context and
> handle any interrupt (including IPIs), as the result it cannot handle
> SMP call for stack dump.
>
> This patch is to enable coresight debug module, so firstly this driver
> is to bind apb clock for debug module and this is to ensure the debug
> module can be accessed from program or external debugger. And the driver
> uses sample-based registers for debug purpose, e.g. when system triggers
> panic, the driver will dump program counter and combined context
> registers (EDCIDSR, EDVIDSR); by parsing context registers so can
> quickly get to know CPU secure state, exception level, etc.
>
> Some of the debug module registers are located in CPU power domain, so
> this requires the CPU power domain stays on when access related debug
> registers, but the power management for CPU power domain is quite
> dependent on SoC integration for power management. For the platforms
> which with sane power controller implementations, this driver follows
> the method to set EDPRCR to try to pull the CPU out of low power state
> and then set 'no power down request' bit so the CPU has no chance to
> lose power.
>
> If the SoC has not followed up this design well for power management
> controller, the user should use the command line parameter or sysfs
> to constrain all or partial idle states to ensure the CPU power
> domain is enabled and access coresight CPU debug component safely.
>
> Signed-off-by: Leo Yan <leo.yan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
This is coming along well - a few comment below. In your next revision please
add GKH to the 'To' list.
Thanks,
Mathieu
> ---
> drivers/hwtracing/coresight/Kconfig | 14 +
> drivers/hwtracing/coresight/Makefile | 1 +
> drivers/hwtracing/coresight/coresight-cpu-debug.c | 667 ++++++++++++++++++++++
> 3 files changed, 682 insertions(+)
> create mode 100644 drivers/hwtracing/coresight/coresight-cpu-debug.c
>
> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> index 130cb21..8d55d6d 100644
> --- a/drivers/hwtracing/coresight/Kconfig
> +++ b/drivers/hwtracing/coresight/Kconfig
> @@ -89,4 +89,18 @@ config CORESIGHT_STM
> logging useful software events or data coming from various entities
> in the system, possibly running different OSs
>
> +config CORESIGHT_CPU_DEBUG
> + tristate "CoreSight CPU Debug driver"
> + depends on ARM || ARM64
> + depends on DEBUG_FS
> + help
> + This driver provides support for coresight debugging module. This
> + is primarily used to dump sample-based profiling registers when
> + system triggers panic, the driver will parse context registers so
> + can quickly get to know program counter (PC), secure state,
> + exception level, etc. Before use debugging functionality, platform
> + needs to ensure the clock domain and power domain are enabled
> + properly, please refer Documentation/trace/coresight-cpu-debug.txt
> + for detailed description and the example for usage.
> +
> endif
> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
> index af480d9..433d590 100644
> --- a/drivers/hwtracing/coresight/Makefile
> +++ b/drivers/hwtracing/coresight/Makefile
> @@ -16,3 +16,4 @@ obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o \
> coresight-etm4x-sysfs.o
> obj-$(CONFIG_CORESIGHT_QCOM_REPLICATOR) += coresight-replicator-qcom.o
> obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o
> +obj-$(CONFIG_CORESIGHT_CPU_DEBUG) += coresight-cpu-debug.o
> diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> new file mode 100644
> index 0000000..8470e31
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> @@ -0,0 +1,667 @@
> +/*
> + * Copyright (c) 2017 Linaro Limited. All rights reserved.
> + *
> + * Author: Leo Yan <leo.yan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +#include <linux/amba/bus.h>
> +#include <linux/coresight.h>
> +#include <linux/cpu.h>
> +#include <linux/debugfs.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/pm_qos.h>
> +#include <linux/slab.h>
> +#include <linux/smp.h>
> +#include <linux/types.h>
> +#include <linux/uaccess.h>
> +
> +#include "coresight-priv.h"
> +
> +#define EDPCSR 0x0A0
> +#define EDCIDSR 0x0A4
> +#define EDVIDSR 0x0A8
> +#define EDPCSR_HI 0x0AC
> +#define EDOSLAR 0x300
> +#define EDPRCR 0x310
> +#define EDPRSR 0x314
> +#define EDDEVID1 0xFC4
> +#define EDDEVID 0xFC8
> +
> +#define EDPCSR_PROHIBITED 0xFFFFFFFF
> +
> +/* bits definition for EDPCSR */
> +#define EDPCSR_THUMB BIT(0)
> +#define EDPCSR_ARM_INST_MASK GENMASK(31, 2)
> +#define EDPCSR_THUMB_INST_MASK GENMASK(31, 1)
> +
> +/* bits definition for EDPRCR */
> +#define EDPRCR_COREPURQ BIT(3)
> +#define EDPRCR_CORENPDRQ BIT(0)
> +
> +/* bits definition for EDPRSR */
> +#define EDPRSR_DLK BIT(6)
> +#define EDPRSR_PU BIT(0)
> +
> +/* bits definition for EDVIDSR */
> +#define EDVIDSR_NS BIT(31)
> +#define EDVIDSR_E2 BIT(30)
> +#define EDVIDSR_E3 BIT(29)
> +#define EDVIDSR_HV BIT(28)
> +#define EDVIDSR_VMID GENMASK(7, 0)
> +
> +/*
> + * bits definition for EDDEVID1:PSCROffset
> + *
> + * NOTE: armv8 and armv7 have different definition for the register,
> + * so consolidate the bits definition as below:
> + *
> + * 0b0000 - Sample offset applies based on the instruction state, we
> + * rely on EDDEVID to check if EDPCSR is implemented or not
> + * 0b0001 - No offset applies.
> + * 0b0010 - No offset applies, but do not use in AArch32 mode
> + *
> + */
> +#define EDDEVID1_PCSR_OFFSET_MASK GENMASK(3, 0)
> +#define EDDEVID1_PCSR_OFFSET_INS_SET (0x0)
> +#define EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 (0x2)
> +
> +/* bits definition for EDDEVID */
> +#define EDDEVID_PCSAMPLE_MODE GENMASK(3, 0)
> +#define EDDEVID_IMPL_EDPCSR (0x1)
> +#define EDDEVID_IMPL_EDPCSR_EDCIDSR (0x2)
> +#define EDDEVID_IMPL_FULL (0x3)
> +
> +#define DEBUG_WAIT_SLEEP 1000
> +#define DEBUG_WAIT_TIMEOUT 32000
> +
> +struct debug_drvdata {
> + void __iomem *base;
> e struct device *dev;
> + int cpu;
> +
> + bool edpcsr_present;
> + bool edcidsr_present;
> + bool edvidsr_present;
> + bool pc_has_offset;
> +
> + u32 edpcsr;
> + u32 edpcsr_hi;
> + u32 edprsr;
> + u32 edvidsr;
> + u32 edcidsr;
> +};
> +
> +static DEFINE_MUTEX(debug_lock);
> +static DEFINE_PER_CPU(struct debug_drvdata *, debug_drvdata);
> +static int debug_count;
> +static struct dentry *debug_debugfs_dir;
> +
> +static bool debug_enable;
> +module_param_named(enable, debug_enable, bool, 0600);
> +MODULE_PARM_DESC(enable, "Knob to enable debug functionality "
> + "(default is 0, which means is disabled by default)");
For this driver we have a debugFS interface so I question the validity of a
kernel module parameter. Other than adding complexity to the code it offers no
real added value. If a user is to insmod a module, it is just as easy to switch
on the functionality using debugFS in a second step.
> +
> +static void debug_os_unlock(struct debug_drvdata *drvdata)
> +{
> + /* Unlocks the debug registers */
> + writel_relaxed(0x0, drvdata->base + EDOSLAR);
Here the wmb is to make sure reordering (either at compile time or in the CPU)
doesn't happend. You need a comment here otherwise checkpatch will complain.
Speaking of which, did you run this through checkpatch?
> + wmb();
> +}
> +
> +/*
> + * According to ARM DDI 0487A.k, before access external debug
> + * registers should firstly check the access permission; if any
> + * below condition has been met then cannot access debug
> + * registers to avoid lockup issue:
> + *
> + * - CPU power domain is powered off;
> + * - The OS Double Lock is locked;
> + *
> + * By checking EDPRSR can get to know if meet these conditions.
> + */
> +static bool debug_access_permitted(struct debug_drvdata *drvdata)
> +{
> + /* CPU is powered off */
> + if (!(drvdata->edprsr & EDPRSR_PU))
> + return false;
> +
> + /* The OS Double Lock is locked */
> + if (drvdata->edprsr & EDPRSR_DLK)
> + return false;
> +
> + return true;
> +}
> +
> +static void debug_force_cpu_powered_up(struct debug_drvdata *drvdata)
> +{
> + bool retried = false;
> + u32 edprcr;
> +
> +try_again:
> +
> + /*
> + * Send request to power management controller and assert
> + * DBGPWRUPREQ signal; if power management controller has
> + * sane implementation, it should enable CPU power domain
> + * in case CPU is in low power state.
> + */
> + edprcr = readl_relaxed(drvdata->base + EDPRCR);
> + edprcr |= EDPRCR_COREPURQ;
> + writel_relaxed(edprcr, drvdata->base + EDPRCR);
> +
> + /* Wait for CPU to be powered up (timeout~=32ms) */
> + if (readx_poll_timeout_atomic(readl_relaxed, drvdata->base + EDPRSR,
> + drvdata->edprsr, (drvdata->edprsr & EDPRSR_PU),
> + DEBUG_WAIT_SLEEP, DEBUG_WAIT_TIMEOUT)) {
> + /*
> + * Unfortunately the CPU cannot be powered up, so return
> + * back and later has no permission to access other
> + * registers. For this case, should disable CPU low power
> + * states to ensure CPU power domain is enabled!
> + */
> + pr_err("%s: power up request for CPU%d failed\n",
> + __func__, drvdata->cpu);
> + return;
> + }
> +
> + /*
> + * At this point the CPU is powered up, so set the no powerdown
> + * request bit so we don't lose power and emulate power down.
> + */
> + edprcr = readl_relaxed(drvdata->base + EDPRCR);
> + edprcr |= EDPRCR_COREPURQ | EDPRCR_CORENPDRQ;
> + writel_relaxed(edprcr, drvdata->base + EDPRCR);
> +
> + drvdata->edprsr = readl_relaxed(drvdata->base + EDPRSR);
> +
> + /* Bail out if CPU is powered up */
> + if (likely(drvdata->edprsr & EDPRSR_PU))
> + return;
/* The core power domain got switched off on use, try again */
if (unlikely(!drvdata->edprsr & EDPRSR_PU))
goto try_again;
I understand you don't want to introduce a infinite loop but if that happens
here, something else has gone very wrong. The above readx_poll_timeout_atomic
loop should take care of bailing out in case of problems. That way you also get
to rid of the retried variable and the code is more simple.
> +
> + /*
> + * Handle race condition if CPU has been waken up but it sleeps
> + * again if EDPRCR_CORENPDRQ has been flipped, so try to run
> + * waken flow one more time.
> + */
> + if (!retried) {
> + retried = true;
> + goto try_again;
> + }
> +}
> +
> +static void debug_read_regs(struct debug_drvdata *drvdata)
> +{
> + u32 save_edprcr;
> +
> + CS_UNLOCK(drvdata->base);
> +
> + /* Unlock os lock */
> + debug_os_unlock(drvdata);
> +
> + /* Save EDPRCR register */
> + save_edprcr = readl_relaxed(drvdata->base + EDPRCR);
> +
> + /*
> + * Ensure CPU power domain is enabled to let registers
> + * are accessiable.
> + */
> + debug_force_cpu_powered_up(drvdata);
> +
> + if (!debug_access_permitted(drvdata))
> + goto out;
> +
> + drvdata->edpcsr = readl_relaxed(drvdata->base + EDPCSR);
> +
> + /*
> + * As described in ARM DDI 0487A.k, if the processing
> + * element (PE) is in debug state, or sample-based
> + * profiling is prohibited, EDPCSR reads as 0xFFFFFFFF;
> + * EDCIDSR, EDVIDSR and EDPCSR_HI registers also become
> + * UNKNOWN state. So directly bail out for this case.
> + */
> + if (drvdata->edpcsr == EDPCSR_PROHIBITED)
> + goto out;
> +
> + /*
> + * A read of the EDPCSR normally has the side-effect of
> + * indirectly writing to EDCIDSR, EDVIDSR and EDPCSR_HI;
> + * at this point it's safe to read value from them.
> + */
> + if (IS_ENABLED(CONFIG_64BIT))
> + drvdata->edpcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI);
> +
> + if (drvdata->edcidsr_present)
> + drvdata->edcidsr = readl_relaxed(drvdata->base + EDCIDSR);
> +
> + if (drvdata->edvidsr_present)
> + drvdata->edvidsr = readl_relaxed(drvdata->base + EDVIDSR);
> +
> +out:
> + /* Restore EDPRCR register */
> + writel_relaxed(save_edprcr, drvdata->base + EDPRCR);
> +
> + CS_LOCK(drvdata->base);
> +}
> +
> +static unsigned long debug_adjust_pc(struct debug_drvdata *drvdata)
> +{
> + unsigned long arm_inst_offset = 0, thumb_inst_offset = 0;
> + unsigned long pc;
> +
> + if (IS_ENABLED(CONFIG_64BIT))
> + return (unsigned long)drvdata->edpcsr_hi << 32 |
> + (unsigned long)drvdata->edpcsr;
> +
> + pc = (unsigned long)drvdata->edpcsr;
> +
> + if (drvdata->pc_has_offset) {
> + arm_inst_offset = 8;
> + thumb_inst_offset = 4;
> + }
> +
> + /* Handle thumb instruction */
> + if (pc & EDPCSR_THUMB) {
> + pc = (pc & EDPCSR_THUMB_INST_MASK) - thumb_inst_offset;
> + return pc;
> + }
> +
> + /*
> + * Handle arm instruction offset, if the arm instruction
> + * is not 4 byte alignment then it's possible the case
> + * for implementation defined; keep original value for this
> + * case and print info for notice.
> + */
> + if (pc & BIT(1))
> + pr_emerg("Instruction offset is implementation defined\n");
> + else
> + pc = (pc & EDPCSR_ARM_INST_MASK) - arm_inst_offset;
> +
> + return pc;
> +}
> +
> +static void debug_dump_regs(struct debug_drvdata *drvdata)
> +{
> + unsigned long pc;
> +
> + pr_emerg("\tEDPRSR: %08x (Power:%s DLK:%s)\n", drvdata->edprsr,
> + drvdata->edprsr & EDPRSR_PU ? "On" : "Off",
> + drvdata->edprsr & EDPRSR_DLK ? "Lock" : "Unlock");
> +
> + if (!debug_access_permitted(drvdata)) {
> + pr_emerg("No permission to access debug registers!\n");
> + return;
> + }
> +
> + if (drvdata->edpcsr == EDPCSR_PROHIBITED) {
> + pr_emerg("CPU is in Debug state or profiling is prohibited!\n");
> + return;
> + }
> +
> + pc = debug_adjust_pc(drvdata);
> + pr_emerg("\tEDPCSR: [<%p>] %pS\n", (void *)pc, (void *)pc);
> +
> + if (drvdata->edcidsr_present)
> + pr_emerg("\tEDCIDSR: %08x\n", drvdata->edcidsr);
> +
> + if (drvdata->edvidsr_present)
> + pr_emerg("\tEDVIDSR: %08x (State:%s Mode:%s Width:%dbits VMID:%x)\n",
> + drvdata->edvidsr,
> + drvdata->edvidsr & EDVIDSR_NS ? "Non-secure" : "Secure",
> + drvdata->edvidsr & EDVIDSR_E3 ? "EL3" :
> + (drvdata->edvidsr & EDVIDSR_E2 ? "EL2" : "EL1/0"),
> + drvdata->edvidsr & EDVIDSR_HV ? 64 : 32,
> + drvdata->edvidsr & (u32)EDVIDSR_VMID);
> +}
> +
> +static void debug_init_arch_data(void *info)
> +{
> + struct debug_drvdata *drvdata = info;
> + u32 mode, pcsr_offset;
> + u32 eddevid, eddevid1;
> +
> + CS_UNLOCK(drvdata->base);
> +
> + /* Read device info */
> + eddevid = readl_relaxed(drvdata->base + EDDEVID);
> + eddevid1 = readl_relaxed(drvdata->base + EDDEVID1);
> +
> + CS_LOCK(drvdata->base);
> +
> + /* Parse implementation feature */
> + mode = eddevid & EDDEVID_PCSAMPLE_MODE;
> + pcsr_offset = eddevid1 & EDDEVID1_PCSR_OFFSET_MASK;
> +
> + drvdata->edpcsr_present = false;
> + drvdata->edcidsr_present = false;
> + drvdata->edvidsr_present = false;
> + drvdata->pc_has_offset = false;
> +
> + switch (mode) {
> + case EDDEVID_IMPL_FULL:
> + drvdata->edvidsr_present = true;
> + /* Fall through */
> + case EDDEVID_IMPL_EDPCSR_EDCIDSR:
> + drvdata->edcidsr_present = true;
> + /* Fall through */
> + case EDDEVID_IMPL_EDPCSR:
> + /*
> + * In ARM DDI 0487A.k, the EDDEVID1.PCSROffset is used to
> + * define if has the offset for PC sampling value; if read
> + * back EDDEVID1.PCSROffset == 0x2, then this means the debug
> + * module does not sample the instruction set state when
> + * armv8 CPU in AArch32 state.
> + */
> + drvdata->edpcsr_present = (IS_ENABLED(CONFIG_64BIT) ||
> + (pcsr_offset != EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32));
> +
> + drvdata->pc_has_offset =
> + (pcsr_offset == EDDEVID1_PCSR_OFFSET_INS_SET);
> + break;
> + default:
> + break;
> + }
> +}
> +
> +/*
> + * Dump out information on panic.
> + */
> +static int debug_notifier_call(struct notifier_block *self,
> + unsigned long v, void *p)
> +{
> + int cpu;
> + struct debug_drvdata *drvdata;
> +
> + pr_emerg("ARM external debug module:\n");
> +
> + for_each_possible_cpu(cpu) {
> + drvdata = per_cpu(debug_drvdata, cpu);
> + if (!drvdata)
> + continue;
> +
> + pr_emerg("CPU[%d]:\n", drvdata->cpu);
> +
> + debug_read_regs(drvdata);
> + debug_dump_regs(drvdata);
> + }
> +
> + return 0;
> +}
> +
> +static struct notifier_block debug_notifier = {
> + .notifier_call = debug_notifier_call,
> +};
> +
> +static int debug_enable_func(void)
> +{
> + struct debug_drvdata *drvdata;
> + int cpu;
> +
> + for_each_possible_cpu(cpu) {
> + drvdata = per_cpu(debug_drvdata, cpu);
> + if (!drvdata)
> + continue;
> +
> + pm_runtime_get_sync(drvdata->dev);
> + }
> +
> + return atomic_notifier_chain_register(&panic_notifier_list,
> + &debug_notifier);
> +}
> +
> +static int debug_disable_func(void)
> +{
> + struct debug_drvdata *drvdata;
> + int cpu;
> +
> + for_each_possible_cpu(cpu) {
> + drvdata = per_cpu(debug_drvdata, cpu);
> + if (!drvdata)
> + continue;
> +
> + pm_runtime_put(drvdata->dev);
> + }
> +
> + return atomic_notifier_chain_unregister(&panic_notifier_list,
> + &debug_notifier);
> +}
> +
> +static ssize_t debug_func_knob_write(struct file *f,
> + const char __user *buf, size_t count, loff_t *ppos)
> +{
> + u8 val;
> + int ret;
> +
> + ret = kstrtou8_from_user(buf, count, 2, &val);
> + if (ret)
> + return ret;
> +
> + mutex_lock(&debug_lock);
> +
> + if (val == debug_enable)
> + goto out;
> +
> + if (val)
> + ret = debug_enable_func();
> + else
> + ret = debug_disable_func();
I don't think you need to install the handler every time the functionality is
switched on/off. I suggest to install the handler at boot time (or module
insertion time) and in the notifier handler, check the debug_enable flag before
moving on with the output.
> +
> + if (ret) {
> + pr_err("%s: unable to %s debug function: %d\n",
> + __func__, val ? "enable" : "disable", ret);
> + goto err;
> + }
> +
> + debug_enable = val;
Using a true/false value is probably better here. That way you don't end up
with miscellaneous values in debugFS.
> +out:
> + ret = count;
> +err:
> + mutex_unlock(&debug_lock);
> + return ret;
> +}
> +
> +static ssize_t debug_func_knob_read(struct file *f,
> + char __user *ubuf, size_t count, loff_t *ppos)
> +{
> + ssize_t ret;
> + char buf[2];
> +
> + mutex_lock(&debug_lock);
> +
> + buf[0] = '0' + debug_enable;
> + buf[1] = '\n';
snprintf()
> + ret = simple_read_from_buffer(ubuf, count, ppos, buf, sizeof(buf));
> +
> + mutex_unlock(&debug_lock);
> + return ret;
> +}
> +
> +static const struct file_operations debug_func_knob_fops = {
> + .open = simple_open,
> + .read = debug_func_knob_read,
> + .write = debug_func_knob_write,
> +};
> +
> +static int debug_func_init(void)
> +{
> + struct dentry *file;
> + int ret;
> +
> + /* Create debugfs node */
> + debug_debugfs_dir = debugfs_create_dir("coresight_cpu_debug", NULL);
> + if (!debug_debugfs_dir) {
> + pr_err("%s: unable to create debugfs directory\n", __func__);
> + return -ENOMEM;
> + }
> +
> + file = debugfs_create_file("enable", S_IRUGO | S_IWUSR,
> + debug_debugfs_dir, NULL, &debug_func_knob_fops);
Please align this properly.
> + if (!file) {
> + pr_err("%s: unable to create enable knob file\n", __func__);
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + /* Use sysfs node to enable functionality */
> + if (!debug_enable)
> + return 0;
> +
> + /* Register function to be called for panic */
> + ret = atomic_notifier_chain_register(&panic_notifier_list,
> + &debug_notifier);
> + if (ret) {
> + pr_err("%s: unable to register notifier: %d\n",
> + __func__, ret);
> + goto err;
> + }
> +
> + return 0;
> +
> +err:
> + debugfs_remove_recursive(debug_debugfs_dir);
> + return ret;
> +}
> +
> +static void debug_func_exit(void)
> +{
> + debugfs_remove_recursive(debug_debugfs_dir);
> +
> + /* Unregister panic notifier callback */
> + if (debug_enable)
> + atomic_notifier_chain_unregister(&panic_notifier_list,
> + &debug_notifier);
> +}
> +
> +static int debug_probe(struct amba_device *adev, const struct amba_id *id)
> +{
> + void __iomem *base;
> + struct device *dev = &adev->dev;
> + struct debug_drvdata *drvdata;
> + struct resource *res = &adev->res;
> + struct device_node *np = adev->dev.of_node;
> + int ret;
> +
> + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> + if (!drvdata)
> + return -ENOMEM;
> +
> + drvdata->cpu = np ? of_coresight_get_cpu(np) : 0;
> + if (per_cpu(debug_drvdata, drvdata->cpu)) {
> + dev_err(dev, "CPU%d drvdata has been initialized\n",
> + drvdata->cpu);
> + return -EBUSY;
> + }
> +
> + drvdata->dev = &adev->dev;
> + amba_set_drvdata(adev, drvdata);
> +
> + /* Validity for the resource is already checked by the AMBA core */
> + base = devm_ioremap_resource(dev, res);
> + if (IS_ERR(base))
> + return PTR_ERR(base);
> +
> + drvdata->base = base;
> +
> + get_online_cpus();
> + per_cpu(debug_drvdata, drvdata->cpu) = drvdata;
> + ret = smp_call_function_single(drvdata->cpu,
> + debug_init_arch_data, drvdata, 1);
> + put_online_cpus();
> +
> + if (ret) {
> + dev_err(dev, "CPU%d debug arch init failed\n", drvdata->cpu);
> + goto err;
> + }
> +
> + if (!drvdata->edpcsr_present) {
> + dev_err(dev, "CPU%d sample-based profiling isn't implemented\n",
> + drvdata->cpu);
> + ret = -ENXIO;
> + goto err;
> + }
> +
> + if (!debug_count++) {
> + ret = debug_func_init();
> + if (ret)
> + goto err_func_init;
> + }
> +
> + if (!debug_enable)
> + pm_runtime_put(dev);
> +
> + dev_info(dev, "Coresight debug-CPU%d initialized\n", drvdata->cpu);
> + return 0;
> +
> +err_func_init:
> + debug_count--;
> +err:
> + per_cpu(debug_drvdata, drvdata->cpu) = NULL;
> + return ret;
> +}
> +
> +static int debug_remove(struct amba_device *adev)
> +{
> + struct device *dev = &adev->dev;
> + struct debug_drvdata *drvdata = amba_get_drvdata(adev);
> +
> + per_cpu(debug_drvdata, drvdata->cpu) = NULL;
> +
> + if (debug_enable)
> + pm_runtime_put(dev);
> +
> + if (!--debug_count)
> + debug_func_exit();
> +
> + return 0;
> +}
> +
> +static struct amba_id debug_ids[] = {
> + { /* Debug for Cortex-A53 */
> + .id = 0x000bbd03,
> + .mask = 0x000fffff,
> + },
> + { /* Debug for Cortex-A57 */
> + .id = 0x000bbd07,
> + .mask = 0x000fffff,
> + },
> + { /* Debug for Cortex-A72 */
> + .id = 0x000bbd08,
> + .mask = 0x000fffff,
> + },
> + { 0, 0 },
> +};
> +
> +static struct amba_driver debug_driver = {
> + .drv = {
> + .name = "coresight-cpu-debug",
> + .suppress_bind_attrs = true,
> + },
> + .probe = debug_probe,
> + .remove = debug_remove,
> + .id_table = debug_ids,
> +};
> +
> +module_amba_driver(debug_driver);
> +
> +MODULE_AUTHOR("Leo Yan <leo.yan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>");
> +MODULE_DESCRIPTION("ARM Coresight CPU Debug Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.7.4
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] drm: rcar-du: Document the vsps property in the DT bindings
From: Geert Uytterhoeven @ 2017-04-18 17:36 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Laurent Pinchart, Sergei Shtylyov, devicetree@vger.kernel.org,
DRI Development, Linux-Renesas
In-Reply-To: <8105647.ANSr0hWnZy@avalon>
Hi Laurent,
On Fri, Mar 31, 2017 at 11:19 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Monday 27 Mar 2017 13:05:48 Geert Uytterhoeven wrote:
>> On Mon, Mar 27, 2017 at 11:56 AM, Laurent Pinchart wrote:
>> > The property is used by the driver but is missing from the DT bindings.
>> > Document it.
>> >
>> > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
>> > Signed-off-by: Laurent Pinchart
>> > <laurent.pinchart+renesas@ideasonboard.com>
>> > ---
>> >
>> > Documentation/devicetree/bindings/display/renesas,du.txt | 5 +++++
>> > 1 file changed, 5 insertions(+)
>> >
>> > diff --git a/Documentation/devicetree/bindings/display/renesas,du.txt
>> > b/Documentation/devicetree/bindings/display/renesas,du.txt index
>> > 1a02f099a0ff..cf34893a1b53 100644
>> > --- a/Documentation/devicetree/bindings/display/renesas,du.txt
>> > +++ b/Documentation/devicetree/bindings/display/renesas,du.txt
>> >
>> > @@ -36,6 +36,11 @@ Required Properties:
>> > When supplied they must be named "dclkin.x" with "x" being the
>> > input
>> > clock numerical index.
>> >
>> > +Optional Properties:
>> > +
>> > + - vsps: A list of phandles to the VSP nodes that handle the memory
>> > + interfaces for the DU channels (Gen3 only).
>>
>> ", one per channel"?
>>
>> Required for Gen3, optional for Gen2? (cfr. Sergei's patches).
>
> How about making it mandatory on Gen2 as well ? The VSPs are there, even if
> the driver doesn't use them, it makes sense to describe the connection. Of
> course the driver will treat the property as optional for backward
> compatibility.
Now it's mandatory, the vsps property should be present in the example, too.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* RE: [PATCH v3 5/7] doc_rst: media: New SDR formats PC16, PC18 & PC20
From: Ramesh Shanmugasundaram @ 2017-04-18 17:13 UTC (permalink / raw)
To: Laurent Pinchart
Cc: robh+dt@kernel.org, mark.rutland@arm.com, mchehab@kernel.org,
hverkuil@xs4all.nl, sakari.ailus@linux.intel.com, crope@iki.fi,
Chris Paterson, geert+renesas@glider.be,
linux-media@vger.kernel.org, devicetree@vger.kernel.org,
linux-renesas-soc@vger.kernel.org
In-Reply-To: <213133284.36Jzg1zrIM@avalon>
Hi Laurent,
Thanks for the review comments.
> On Tuesday 07 Feb 2017 15:02:35 Ramesh Shanmugasundaram wrote:
> > This patch adds documentation for the three new SDR formats
> >
> > V4L2_SDR_FMT_PCU16BE
> > V4L2_SDR_FMT_PCU18BE
> > V4L2_SDR_FMT_PCU20BE
> >
> > Signed-off-by: Ramesh Shanmugasundaram
> > <ramesh.shanmugasundaram@bp.renesas.com> ---
> > .../media/uapi/v4l/pixfmt-sdr-pcu16be.rst | 55
> +++++++++++++++++++
> > .../media/uapi/v4l/pixfmt-sdr-pcu18be.rst | 55
> +++++++++++++++++++
> > .../media/uapi/v4l/pixfmt-sdr-pcu20be.rst | 54
> +++++++++++++++++++
> > Documentation/media/uapi/v4l/sdr-formats.rst | 3 ++
> > 4 files changed, 167 insertions(+)
> > create mode 100644
> > Documentation/media/uapi/v4l/pixfmt-sdr-pcu16be.rst
> > create mode 100644
> > Documentation/media/uapi/v4l/pixfmt-sdr-pcu18be.rst
> > create mode 100644
> > Documentation/media/uapi/v4l/pixfmt-sdr-pcu20be.rst
> >
> > diff --git a/Documentation/media/uapi/v4l/pixfmt-sdr-pcu16be.rst
> > b/Documentation/media/uapi/v4l/pixfmt-sdr-pcu16be.rst new file mode
> > 100644 index 0000000..2de1b1a
> > --- /dev/null
> > +++ b/Documentation/media/uapi/v4l/pixfmt-sdr-pcu16be.rst
> > @@ -0,0 +1,55 @@
> > +.. -*- coding: utf-8; mode: rst -*-
> > +
> > +.. _V4L2-SDR-FMT-PCU16BE:
> > +
> > +******************************
> > +V4L2_SDR_FMT_PCU16BE ('PC16')
> > +******************************
> > +
> > +Planar complex unsigned 16-bit big endian IQ sample
> > +
> > +Description
> > +===========
> > +
> > +This format contains a sequence of complex number samples. Each
> > +complex number consist of two parts called In-phase and Quadrature
> > +(IQ). Both I and Q are represented as a 16 bit unsigned big endian
> > +number stored in
> > +32 bit space. The remaining unused bits within the 32 bit space will
> > +be padded with 0. I value starts first and Q value starts at an
> > +offset equalling half of the buffer size (i.e.) offset =
> > +buffersize/2. Out of the 16 bits, bit 15:2 (14 bit) is data and bit
> > +1:0 (2 bit) can be any value.
>
> This sounds very strange to me. Are the two lower bits always random ?
> What is that used for ?
It could be zeros or it could be status bits in case of MAX2175 (if enabled). I mentioned any value because the user app does not have any assumptions on these bits value.
>
> > +**Byte Order.**
> > +Each cell is one byte.
> > +
> > +.. flat-table::
> > + :header-rows: 1
> > + :stub-columns: 0
> > +
> > + * - Offset:
> > + - Byte B0
> > + - Byte B1
> > + - Byte B2
> > + - Byte B3
> > + * - start + 0:
> > + - I'\ :sub:`0[13:6]`
> > + - I'\ :sub:`0[5:0]; B1[1:0]=pad`
> > + - pad
> > + - pad
> > + * - start + 4:
> > + - I'\ :sub:`1[13:6]`
> > + - I'\ :sub:`1[5:0]; B1[1:0]=pad`
> > + - pad
> > + - pad
> > + * - ...
> > + * - start + offset:
> > + - Q'\ :sub:`0[13:6]`
> > + - Q'\ :sub:`0[5:0]; B1[1:0]=pad`
> > + - pad
> > + - pad
> > + * - start + offset + 4:
> > + - Q'\ :sub:`1[13:6]`
> > + - Q'\ :sub:`1[5:0]; B1[1:0]=pad`
> > + - pad
> > + - pad
> > diff --git a/Documentation/media/uapi/v4l/pixfmt-sdr-pcu18be.rst
> > b/Documentation/media/uapi/v4l/pixfmt-sdr-pcu18be.rst new file mode
> > 100644 index 0000000..da8b26b
> > --- /dev/null
> > +++ b/Documentation/media/uapi/v4l/pixfmt-sdr-pcu18be.rst
> > @@ -0,0 +1,55 @@
> > +.. -*- coding: utf-8; mode: rst -*-
> > +
> > +.. _V4L2-SDR-FMT-PCU18BE:
> > +
> > +******************************
> > +V4L2_SDR_FMT_PCU18BE ('PC18')
> > +******************************
> > +
> > +Planar complex unsigned 18-bit big endian IQ sample
> > +
> > +Description
> > +===========
> > +
> > +This format contains a sequence of complex number samples. Each
> > +complex number consist of two parts called In-phase and Quadrature
> > +(IQ). Both I and Q are represented as a 18 bit unsigned big endian
> > +number stored in
> > +32 bit space. The remaining unused bits within the 32 bit space will
> > +be padded with 0. I value starts first and Q value starts at an
> > +offset equalling half of the buffer size (i.e.) offset =
> > +buffersize/2. Out of the 18 bits, bit 17:2 (16 bit) is data and bit
> > +1:0 (2 bit) can be any value.
> > +
> > +**Byte Order.**
> > +Each cell is one byte.
> > +
> > +.. flat-table::
> > + :header-rows: 1
> > + :stub-columns: 0
> > +
> > + * - Offset:
> > + - Byte B0
> > + - Byte B1
> > + - Byte B2
> > + - Byte B3
> > + * - start + 0:
> > + - I'\ :sub:`0[17:10]`
> > + - I'\ :sub:`0[9:2]`
> > + - I'\ :sub:`0[1:0]; B2[5:0]=pad`
> > + - pad
> > + * - start + 4:
> > + - I'\ :sub:`1[17:10]`
> > + - I'\ :sub:`1[9:2]`
> > + - I'\ :sub:`1[1:0]; B2[5:0]=pad`
> > + - pad
> > + * - ...
> > + * - start + offset:
> > + - Q'\ :sub:`0[17:10]`
> > + - Q'\ :sub:`0[9:2]`
> > + - Q'\ :sub:`0[1:0]; B2[5:0]=pad`
> > + - pad
> > + * - start + offset + 4:
> > + - Q'\ :sub:`1[17:10]`
> > + - Q'\ :sub:`1[9:2]`
> > + - Q'\ :sub:`1[1:0]; B2[5:0]=pad`
> > + - pad
> > diff --git a/Documentation/media/uapi/v4l/pixfmt-sdr-pcu20be.rst
> > b/Documentation/media/uapi/v4l/pixfmt-sdr-pcu20be.rst new file mode
> > 100644 index 0000000..5499eed
> > --- /dev/null
> > +++ b/Documentation/media/uapi/v4l/pixfmt-sdr-pcu20be.rst
> > @@ -0,0 +1,54 @@
> > +.. -*- coding: utf-8; mode: rst -*-
> > +.. _V4L2-SDR-FMT-PCU20BE:
> > +
> > +******************************
> > +V4L2_SDR_FMT_PCU20BE ('PC20')
> > +******************************
> > +
> > +Planar complex unsigned 20-bit big endian IQ sample
> > +
> > +Description
> > +===========
> > +
> > +This format contains a sequence of complex number samples. Each
> > +complex number consist of two parts called In-phase and Quadrature
> > +(IQ). Both I and Q are represented as a 20 bit unsigned big endian
> > +number stored in
> > +32 bit space. The remaining unused bits within the 32 bit space will
> > +be padded with 0. I value starts first and Q value starts at an
> > +offset equalling half of the buffer size (i.e.) offset =
> > +buffersize/2. Out of the 20 bits, bit 19:2 (18 bit) is data and bit
> > +1:0 (2 bit) can be any value.
> > +
> > +**Byte Order.**
> > +Each cell is one byte.
> > +
> > +.. flat-table::
> > + :header-rows: 1
> > + :stub-columns: 0
> > +
> > + * - Offset:
> > + - Byte B0
> > + - Byte B1
> > + - Byte B2
> > + - Byte B3
> > + * - start + 0:
> > + - I'\ :sub:`0[19:12]`
> > + - I'\ :sub:`0[11:4]`
> > + - I'\ :sub:`0[3:0]; B2[3:0]=pad`
> > + - pad
> > + * - start + 4:
> > + - I'\ :sub:`1[19:12]`
> > + - I'\ :sub:`1[11:4]`
> > + - I'\ :sub:`1[3:0]; B2[3:0]=pad`
> > + - pad
> > + * - ...
> > + * - start + offset:
> > + - Q'\ :sub:`0[19:12]`
> > + - Q'\ :sub:`0[11:4]`
> > + - Q'\ :sub:`0[3:0]; B2[3:0]=pad`
> > + - pad
> > + * - start + offset + 4:
> > + - Q'\ :sub:`1[19:12]`
> > + - Q'\ :sub:`1[11:4]`
> > + - Q'\ :sub:`1[3:0]; B2[3:0]=pad`
> > + - pad
> > diff --git a/Documentation/media/uapi/v4l/sdr-formats.rst
> > b/Documentation/media/uapi/v4l/sdr-formats.rst index f863c08..2037f5b
> > 100644
> > --- a/Documentation/media/uapi/v4l/sdr-formats.rst
> > +++ b/Documentation/media/uapi/v4l/sdr-formats.rst
> > @@ -17,3 +17,6 @@ These formats are used for :ref:`SDR <sdr>`
> > interface only. pixfmt-sdr-cs08
> > pixfmt-sdr-cs14le
> > pixfmt-sdr-ru12le
> > + pixfmt-sdr-pcu16be
> > + pixfmt-sdr-pcu18be
> > + pixfmt-sdr-pcu20be
Thanks,
Ramesh
^ permalink raw reply
* RE: [PATCH v3 7/7] media: platform: rcar_drif: Add DRIF support
From: Ramesh Shanmugasundaram @ 2017-04-18 17:12 UTC (permalink / raw)
To: Laurent Pinchart
Cc: robh+dt@kernel.org, mark.rutland@arm.com, mchehab@kernel.org,
hverkuil@xs4all.nl, sakari.ailus@linux.intel.com, crope@iki.fi,
Chris Paterson, geert+renesas@glider.be,
linux-media@vger.kernel.org, devicetree@vger.kernel.org,
linux-renesas-soc@vger.kernel.org
In-Reply-To: <2330193.GrgnD4vKf8@avalon>
Hi Laurent,
Many thanks for your time & the review comments. I have agreed to most of the comments and a few need further discussion. Could you please take a look at those?
> On Tuesday 07 Feb 2017 15:02:37 Ramesh Shanmugasundaram wrote:
> > This patch adds Digital Radio Interface (DRIF) support to R-Car Gen3
> SoCs.
> > The driver exposes each instance of DRIF as a V4L2 SDR device. A DRIF
> > device represents a channel and each channel can have one or two
> > sub-channels respectively depending on the target board.
> >
> > DRIF supports only Rx functionality. It receives samples from a RF
> > frontend tuner chip it is interfaced with. The combination of DRIF and
> > the tuner device, which is registered as a sub-device, determines the
> > receive sample rate and format.
> >
> > In order to be compliant as a V4L2 SDR device, DRIF needs to bind with
> > the tuner device, which can be provided by a third party vendor. DRIF
> > acts as a slave device and the tuner device acts as a master
> > transmitting the samples. The driver allows asynchronous binding of a
> > tuner device that is registered as a v4l2 sub-device. The driver can
> > learn about the tuner it is interfaced with based on port endpoint
> > properties of the device in device tree. The V4L2 SDR device inherits
> > the controls exposed by the tuner device.
> >
> > The device can also be configured to use either one or both of the
> > data pins at runtime based on the master (tuner) configuration.
> >
> > Signed-off-by: Ramesh Shanmugasundaram
> > <ramesh.shanmugasundaram@bp.renesas.com>
> > ---
> > drivers/media/platform/Kconfig | 25 +
> > drivers/media/platform/Makefile | 1 +
> > drivers/media/platform/rcar_drif.c | 1534
> > +++++++++++++++++++++++++++++++++
> > 3 files changed, 1560 insertions(+)
> > create mode 100644 drivers/media/platform/rcar_drif.c
>
> [snip]
>
> > diff --git a/drivers/media/platform/rcar_drif.c
> > b/drivers/media/platform/rcar_drif.c new file mode 100644 index
> > 0000000..88950e3
> > --- /dev/null
> > +++ b/drivers/media/platform/rcar_drif.c
> > @@ -0,0 +1,1534 @@
>
> [snip]
>
> > +/*
> > + * The R-Car DRIF is a receive only MSIOF like controller with an
> > + * external master device driving the SCK. It receives data into a
> > +FIFO,
> > + * then this driver uses the SYS-DMAC engine to move the data from
> > + * the device to memory.
> > + *
> > + * Each DRIF channel DRIFx (as per datasheet) contains two internal
> > + * channels DRIFx0 & DRIFx1 within itself with each having its own
> > resources
> > + * like module clk, register set, irq and dma. These internal
> > + channels
> > share
> > + * common CLK & SYNC from master. The two data pins D0 & D1 shall be
> > + * considered to represent the two internal channels. This internal
> > + split
> > + * is not visible to the master device.
> > + *
> > + * Depending on the master device, a DRIF channel can use
> > + * (1) both internal channels (D0 & D1) to receive data in parallel
> > + (or)
> > + * (2) one internal channel (D0 or D1) to receive data
> > + *
> > + * The primary design goal of this controller is to act as Digitial
> > + Radio
>
> s/Digitial/Digital/
Agreed
>
> > + * Interface that receives digital samples from a tuner device. Hence
> > + the
> > + * driver exposes the device as a V4L2 SDR device. In order to
> > + qualify as
> > + * a V4L2 SDR device, it should possess tuner interface as mandated
> > + by the
> > + * framework. This driver expects a tuner driver (sub-device) to bind
> > + * asynchronously with this device and the combined drivers shall
> > + expose
> > + * a V4L2 compliant SDR device. The DRIF driver is independent of the
> > + * tuner vendor.
> > + *
> > + * The DRIF h/w can support I2S mode and Frame start synchronization
> > + pulse
> > mode.
> > + * This driver is tested for I2S mode only because of the
> > + availability of
> > + * suitable master devices. Hence, not all configurable options of
> > + DRIF h/w
> > + * like lsb/msb first, syncdl, dtdl etc. are exposed via DT and I2S
> > defaults
> > + * are used. These can be exposed later if needed after testing.
> > + */
>
> [snip]
>
> > +#define to_rcar_drif_buf_pair(sdr, ch_num,
> > idx) (sdr->ch[!(ch_num)]->buf[idx])
>
> You should enclose both sdr and idx in parenthesis, as they can be
> expressions.
Agreed.
>
> > +
> > +#define for_each_rcar_drif_channel(ch, ch_mask) \
> > + for_each_set_bit(ch, ch_mask, RCAR_DRIF_MAX_CHANNEL)
> > +
> > +static const unsigned int num_hwbufs = 32;
>
> Is there a specific reason to make this a static const instead of a
> #define ?
Just style only. The #define needs a RCAR_DRIF_ prefix and I used this value in few places. The #define makes the statements longer.
>
> > +/* Debug */
> > +static unsigned int debug;
> > +module_param(debug, uint, 0644);
> > +MODULE_PARM_DESC(debug, "activate debug info");
> > +
> > +#define rdrif_dbg(level, sdr, fmt, arg...) \
> > + v4l2_dbg(level, debug, &sdr->v4l2_dev, fmt, ## arg)
> > +
> > +#define rdrif_err(sdr, fmt, arg...) \
> > + dev_err(sdr->v4l2_dev.dev, fmt, ## arg)
> > +
> > +/* Stream formats */
> > +struct rcar_drif_format {
> > + u32 pixelformat;
> > + u32 buffersize;
> > + u32 wdlen;
> > + u32 num_ch;
> > +};
> > +
> > +/* Format descriptions for capture */ static const struct
> > +rcar_drif_format formats[] = {
> > + {
> > + .pixelformat = V4L2_SDR_FMT_PCU16BE,
> > + .buffersize = RCAR_SDR_BUFFER_SIZE,
> > + .wdlen = 16,
> > + .num_ch = 2,
>
> How about aligning the = as in the other lines ?
Agreed
>
> num_ch is always set to 2. Should we remove it for now, and add it back
> later when we'll support single-channel formats ? I think we should avoid
> carrying dead code.
Actually single channel support is tested internally. If single & dual channels are already supported any future change will be just adding the new SDR format only, which could be relatively trivial for this driver. To add new SDR formats today I need to enable appropriate format in the tuner code too, which could be done later on a need basis by us or others.
>
> > + },
> > + {
> > + .pixelformat = V4L2_SDR_FMT_PCU18BE,
> > + .buffersize = RCAR_SDR_BUFFER_SIZE,
> > + .wdlen = 18,
> > + .num_ch = 2,
> > + },
> > + {
> > + .pixelformat = V4L2_SDR_FMT_PCU20BE,
> > + .buffersize = RCAR_SDR_BUFFER_SIZE,
> > + .wdlen = 20,
> > + .num_ch = 2,
> > + },
> > +};
> > +
> > +static const unsigned int NUM_FORMATS = ARRAY_SIZE(formats);
>
> Same question here, can't this be a define ? I think I'd even avoid
> NUM_FORMATS completely and use ARRAY_SIZE(formats) directly in the code,
> to make the boundary check more explicit when iterating over the array.
Agreed.
>
> > +
> > +/* Buffer for a received frame from one or both internal channels */
> > +struct rcar_drif_frame_buf {
> > + /* Common v4l buffer stuff -- must be first */
> > + struct vb2_v4l2_buffer vb;
> > + struct list_head list;
> > +};
> > +
> > +struct rcar_drif_async_subdev {
> > + struct v4l2_subdev *sd;
> > + struct v4l2_async_subdev asd;
> > +};
> > +
> > +/* DMA buffer */
> > +struct rcar_drif_hwbuf {
> > + void *addr; /* CPU-side address */
> > + unsigned int status; /* Buffer status flags */
> > +};
> > +
> > +/* Internal channel */
> > +struct rcar_drif {
> > + struct rcar_drif_sdr *sdr; /* Group device */
> > + struct platform_device *pdev; /* Channel's pdev */
> > + void __iomem *base; /* Base register address */
> > + resource_size_t start; /* I/O resource offset */
> > + struct dma_chan *dmach; /* Reserved DMA channel */
> > + struct clk *clkp; /* Module clock */
> > + struct rcar_drif_hwbuf *buf[RCAR_DRIF_MAX_NUM_HWBUFS]; /* H/W bufs
> */
> > + dma_addr_t dma_handle; /* Handle for all bufs */
> > + unsigned int num; /* Channel number */
> > + bool acting_sdr; /* Channel acting as SDR device */
> > +};
> > +
> > +/* DRIF V4L2 SDR */
> > +struct rcar_drif_sdr {
> > + struct device *dev; /* Platform device */
> > + struct video_device *vdev; /* V4L2 SDR device */
> > + struct v4l2_device v4l2_dev; /* V4L2 device */
> > +
> > + /* Videobuf2 queue and queued buffers list */
> > + struct vb2_queue vb_queue;
> > + struct list_head queued_bufs;
> > + spinlock_t queued_bufs_lock; /* Protects queued_bufs */
> > +
> > + struct mutex v4l2_mutex; /* To serialize ioctls */
> > + struct mutex vb_queue_mutex; /* To serialize streaming ioctls */
> > + struct v4l2_ctrl_handler ctrl_hdl; /* SDR control handler */
> > + struct v4l2_async_notifier notifier; /* For subdev (tuner) */
> > +
> > + /* Current V4L2 SDR format array index */
> > + unsigned int fmt_idx;
>
> Instead of storing the index I would store a pointer to the corresponding
> rcar_drif_format, looking up information about the current format will
> then be easier.
>
Agreed
> > +
> > + /* Device tree SYNC properties */
> > + u32 mdr1;
> > +
> > + /* Internals */
> > + struct rcar_drif *ch[RCAR_DRIF_MAX_CHANNEL]; /* DRIFx0,1 */
> > + unsigned long hw_ch_mask; /* Enabled channels per DT */
> > + unsigned long cur_ch_mask; /* Used channels for an SDR FMT */
> > + u32 num_hw_ch; /* Num of DT enabled channels */
> > + u32 num_cur_ch; /* Num of used channels */
> > + u32 hwbuf_size; /* Each DMA buffer size */
> > + u32 produced; /* Buffers produced by sdr dev */
> > +};
> > +
> > +/* Allocate buffer context */
> > +static int rcar_drif_alloc_bufctxt(struct rcar_drif_sdr *sdr) {
> > + struct rcar_drif_hwbuf *bufctx;
> > + unsigned int i, idx;
> > +
> > + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> > + bufctx = kcalloc(num_hwbufs, sizeof(*bufctx), GFP_KERNEL);
>
> How about embedding the buffer contexts in the rcar_drif structure instead
> of just storing pointers there ? The rcar_drif_hwbuf structure is pretty
> small, it won't make a big difference, and will simplify the code.
Agreed
>
> > + if (!bufctx)
> > + return -ENOMEM;
> > +
> > + for (idx = 0; idx < num_hwbufs; idx++)
> > + sdr->ch[i]->buf[idx] = bufctx + idx;
> > + }
> > + return 0;
> > +}
>
> [snip]
>
> > +/* Release DMA channel */
> > +static void rcar_drif_release_dmachannel(struct rcar_drif_sdr *sdr)
>
> I would name the function rcar_drif_release_dma_channels as it handles all
> channels. Same for rcar_drif_alloc_dma_channels.
Agreed.
>
> > +{
> > + unsigned int i;
> > +
> > + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask)
> > + if (sdr->ch[i]->dmach) {
> > + dma_release_channel(sdr->ch[i]->dmach);
> > + sdr->ch[i]->dmach = NULL;
> > + }
> > +}
> > +
> > +/* Allocate DMA channel */
> > +static int rcar_drif_alloc_dmachannel(struct rcar_drif_sdr *sdr) {
> > + struct dma_slave_config dma_cfg;
> > + unsigned int i;
> > + int ret = -ENODEV;
> > +
> > + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> > + struct rcar_drif *ch = sdr->ch[i];
> > +
> > + ch->dmach = dma_request_slave_channel(&ch->pdev->dev, "rx");
> > + if (!ch->dmach) {
> > + rdrif_err(sdr, "ch%u: dma channel req failed\n", i);
> > + goto dmach_error;
> > + }
> > +
> > + /* Configure slave */
> > + memset(&dma_cfg, 0, sizeof(dma_cfg));
> > + dma_cfg.src_addr = (phys_addr_t)(ch->start +
> RCAR_DRIF_SIRFDR);
> > + dma_cfg.dst_addr = 0;
>
> This isn't needed as you memset the whole structure to 0.
Agreed
>
> > + dma_cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> > + ret = dmaengine_slave_config(ch->dmach, &dma_cfg);
> > + if (ret) {
> > + rdrif_err(sdr, "ch%u: dma slave config failed\n", i);
> > + goto dmach_error;
> > + }
> > + }
> > + return 0;
> > +
> > +dmach_error:
> > + rcar_drif_release_dmachannel(sdr);
> > + return ret;
> > +}
>
> [snip]
>
> > +/* Set MDR defaults */
> > +static inline void rcar_drif_set_mdr1(struct rcar_drif_sdr *sdr) {
> > + unsigned int i;
> > +
> > + /* Set defaults for enabled internal channels */
> > + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> > + /* Refer MSIOF section in manual for this register setting */
> > + writel(RCAR_DRIF_SITMDR1_PCON,
> > + sdr->ch[i]->base + RCAR_DRIF_SITMDR1);
>
> I would create a rcar_drif_write(struct rcar_drif *ch, u32 offset, u32
> data) function, the code will become clearer. Same for the read operation.
Agreed
>
> > + /* Setup MDR1 value */
> > + writel(sdr->mdr1, sdr->ch[i]->base + RCAR_DRIF_SIRMDR1);
> > +
> > + rdrif_dbg(2, sdr, "ch%u: mdr1 = 0x%08x",
> > + i, readl(sdr->ch[i]->base + RCAR_DRIF_SIRMDR1));
>
> Once you've debugged the driver I'm not sure those debugging statements
> are still needed.
I would like to keep this statement please. This single register print would clarify if a user selected DT options and this could be enabled at runtime on a deployed board.
>
> > + }
> > +}
> > +
> > +/* Extract bitlen and wdcnt from given word length */ static int
> > +rcar_drif_convert_wdlen(struct rcar_drif_sdr *sdr,
> > + u32 wdlen, u32 *bitlen, u32 *wdcnt) {
> > + unsigned int i, nr_wds;
> > +
> > + /* FIFO register size is 32 bits */
> > + for (i = 0; i < 32; i++) {
> > + nr_wds = wdlen % (32 - i);
> > + if (nr_wds == 0) {
> > + *bitlen = 32 - i;
> > + *wdcnt = wdlen / *bitlen;
>
> Can't you store the bitlen and wdcnt values in the rcar_drif_format
> structure instead of recomputing them every time ?
Agreed
>
> > + break;
> > + }
> > + }
> > +
> > + /* Sanity check range */
> > + if (i == 32 || !(*bitlen >= 8 && *bitlen <= 32) ||
> > + !(*wdcnt >= 1 && *wdcnt <= 64)) {
> > + rdrif_err(sdr, "invalid wdlen %u configured\n", wdlen);
> > + return -EINVAL;
>
> You shouldn't have invalid wdlen values in the driver. I would remove this
> check as it makes error handling in the caller more complex.
Agreed
>
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/* Set DRIF receive format */
> > +static int rcar_drif_set_format(struct rcar_drif_sdr *sdr) {
> > + u32 bitlen, wdcnt, wdlen;
> > + unsigned int i;
> > + int ret = -EINVAL;
> > +
> > + wdlen = formats[sdr->fmt_idx].wdlen;
> > + rdrif_dbg(2, sdr, "setfmt: idx %u, wdlen %u, num_ch %u\n",
> > + sdr->fmt_idx, wdlen, formats[sdr->fmt_idx].num_ch);
> > +
> > + /* Sanity check */
> > + if (formats[sdr->fmt_idx].num_ch > sdr->num_cur_ch) {
> > + rdrif_err(sdr, "fmt idx %u current ch %u mismatch\n",
> > + sdr->fmt_idx, sdr->num_cur_ch);
> > + return ret;
>
> This should never happen, it should be caught at set format time.
But, this is the set format function?
>
> > + }
> > +
> > + /* Get bitlen & wdcnt from wdlen */
> > + ret = rcar_drif_convert_wdlen(sdr, wdlen, &bitlen, &wdcnt);
> > + if (ret)
> > + return ret;
> > +
> > + /* Setup group, bitlen & wdcnt */
> > + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> > + u32 mdr;
> > +
> > + /* Two groups */
> > + mdr = RCAR_DRIF_MDR_GRPCNT(2) | RCAR_DRIF_MDR_BITLEN(bitlen) |
> > + RCAR_DRIF_MDR_WDCNT(wdcnt);
> > + writel(mdr, sdr->ch[i]->base + RCAR_DRIF_SIRMDR2);
> > +
> > + mdr = RCAR_DRIF_MDR_BITLEN(bitlen) |
> RCAR_DRIF_MDR_WDCNT(wdcnt);
> > + writel(mdr, sdr->ch[i]->base + RCAR_DRIF_SIRMDR3);
> > +
> > + rdrif_dbg(2, sdr, "ch%u: new mdr[2,3] = 0x%08x, 0x%08x\n",
> > + i, readl(sdr->ch[i]->base + RCAR_DRIF_SIRMDR2),
> > + readl(sdr->ch[i]->base + RCAR_DRIF_SIRMDR3));
> > + }
> > + return ret;
> > +}
> > +
> > +/* Release DMA buffers */
> > +static void rcar_drif_release_buf(struct rcar_drif_sdr *sdr) {
> > + unsigned int i;
> > +
> > + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> > + struct rcar_drif *ch = sdr->ch[i];
> > +
> > + /* First entry contains the dma buf ptr */
> > + if (ch->buf[0] && ch->buf[0]->addr) {
> > + dma_free_coherent(&ch->pdev->dev,
> > + sdr->hwbuf_size * num_hwbufs,
> > + ch->buf[0]->addr, ch->dma_handle);
> > + ch->buf[0]->addr = NULL;
> > + }
> > + }
> > +}
> > +
> > +/* Request DMA buffers */
> > +static int rcar_drif_request_buf(struct rcar_drif_sdr *sdr) {
> > + int ret = -ENOMEM;
> > + unsigned int i, j;
> > + void *addr;
> > +
> > + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> > + struct rcar_drif *ch = sdr->ch[i];
> > +
> > + /* Allocate DMA buffers */
> > + addr = dma_alloc_coherent(&ch->pdev->dev,
> > + sdr->hwbuf_size * num_hwbufs,
> > + &ch->dma_handle, GFP_KERNEL);
> > + if (!addr) {
> > + rdrif_err(sdr,
> > + "ch%u: dma alloc failed. num_hwbufs %u size %u\n",
> > + i, num_hwbufs, sdr->hwbuf_size);
> > + goto alloc_error;
> > + }
> > +
> > + /* Split the chunk and populate bufctxt */
> > + for (j = 0; j < num_hwbufs; j++) {
> > + ch->buf[j]->addr = addr + (j * sdr->hwbuf_size);
> > + ch->buf[j]->status = 0;
> > + }
> > + }
> > +
> > + return 0;
> > +
> > +alloc_error:
> > + return ret;
> > +}
> > +
> > +/* Setup vb_queue minimum buffer requirements */ static int
> > +rcar_drif_queue_setup(struct vb2_queue *vq,
> > + unsigned int *num_buffers, unsigned int *num_planes,
> > + unsigned int sizes[], struct device *alloc_devs[]) {
> > + struct rcar_drif_sdr *sdr = vb2_get_drv_priv(vq);
> > +
> > + /* Need at least 16 buffers */
> > + if (vq->num_buffers + *num_buffers < 16)
> > + *num_buffers = 16 - vq->num_buffers;
> > +
> > + *num_planes = 1;
> > + sizes[0] = PAGE_ALIGN(formats[sdr->fmt_idx].buffersize);
> > +
> > + rdrif_dbg(2, sdr, "num_bufs %d sizes[0] %d\n", *num_buffers,
> sizes[0]);
> > + return 0;
> > +}
> > +
> > +/* Enqueue buffer */
> > +static void rcar_drif_buf_queue(struct vb2_buffer *vb) {
> > + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > + struct rcar_drif_sdr *sdr = vb2_get_drv_priv(vb->vb2_queue);
> > + struct rcar_drif_frame_buf *fbuf =
> > + container_of(vbuf, struct rcar_drif_frame_buf, vb);
> > + unsigned long flags;
> > +
> > + rdrif_dbg(2, sdr, "buf_queue idx %u\n", vb->index);
> > + spin_lock_irqsave(&sdr->queued_bufs_lock, flags);
> > + list_add_tail(&fbuf->list, &sdr->queued_bufs);
> > + spin_unlock_irqrestore(&sdr->queued_bufs_lock, flags); }
> > +
> > +/* Get a frame buf from list */
> > +static struct rcar_drif_frame_buf *
> > +rcar_drif_get_fbuf(struct rcar_drif_sdr *sdr) {
> > + struct rcar_drif_frame_buf *fbuf;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&sdr->queued_bufs_lock, flags);
> > + fbuf = list_first_entry_or_null(&sdr->queued_bufs, struct
> > + rcar_drif_frame_buf, list);
> > + if (!fbuf) {
> > + /*
> > + * App is late in enqueing buffers. Samples lost & there will
> > + * be a gap in sequence number when app recovers
> > + */
> > + rdrif_dbg(1, sdr, "\napp late: prod %u\n", sdr->produced);
> > + sdr->produced++; /* Increment the produced count anyway */
> > + spin_unlock_irqrestore(&sdr->queued_bufs_lock, flags);
> > + return NULL;
> > + }
> > + list_del(&fbuf->list);
> > + spin_unlock_irqrestore(&sdr->queued_bufs_lock, flags);
> > +
> > + return fbuf;
> > +}
> > +
> > +static inline bool rcar_drif_buf_pairs_done(struct rcar_drif_hwbuf
> *buf1,
> > + struct rcar_drif_hwbuf *buf2) {
> > + return (buf1->status & buf2->status & RCAR_DRIF_BUF_DONE); }
> > +
> > +/* Channel DMA complete */
> > +static void rcar_drif_channel_complete(struct rcar_drif *ch, u32 idx)
> > +{
> > + u32 str;
> > +
> > + ch->buf[idx]->status |= RCAR_DRIF_BUF_DONE;
> > +
> > + /* Check for DRIF errors */
> > + str = readl(ch->base + RCAR_DRIF_SISTR);
> > + if (unlikely(str & RCAR_DRIF_RFOVF)) {
> > + /* Writing the same clears it */
> > + writel(str, ch->base + RCAR_DRIF_SISTR);
> > +
> > + /* Overflow: some samples are lost */
> > + ch->buf[idx]->status |= RCAR_DRIF_BUF_OVERFLOW;
> > + }
> > +}
> > +
> > +/* Deliver buffer to user */
> > +static void rcar_drif_deliver_buf(struct rcar_drif *ch) {
> > + struct rcar_drif_sdr *sdr = ch->sdr;
> > + u32 idx = sdr->produced % num_hwbufs;
> > + struct rcar_drif_frame_buf *fbuf;
> > + bool overflow = false;
> > +
> > + rcar_drif_channel_complete(ch, idx);
> > +
> > + if (sdr->num_cur_ch == RCAR_DRIF_MAX_CHANNEL) {
> > + struct rcar_drif_hwbuf *bufi, *bufq;
> > +
> > + if (ch->num) {
> > + bufi = to_rcar_drif_buf_pair(sdr, ch->num, idx);
> > + bufq = ch->buf[idx];
> > + } else {
> > + bufi = ch->buf[idx];
> > + bufq = to_rcar_drif_buf_pair(sdr, ch->num, idx);
> > + }
> > +
> > + /* Check if both DMA buffers are done */
> > + if (!rcar_drif_buf_pairs_done(bufi, bufq))
> > + return;
> > +
> > + /* Clear buf done status */
> > + bufi->status &= ~RCAR_DRIF_BUF_DONE;
> > + bufq->status &= ~RCAR_DRIF_BUF_DONE;
> > +
> > + /* Get fbuf */
> > + fbuf = rcar_drif_get_fbuf(sdr);
> > + if (!fbuf)
> > + return;
> > +
> > + memcpy(vb2_plane_vaddr(&fbuf->vb.vb2_buf, 0),
> > + bufi->addr, sdr->hwbuf_size);
> > + memcpy(vb2_plane_vaddr(&fbuf->vb.vb2_buf, 0) + sdr-
> >hwbuf_size,
> > + bufq->addr, sdr->hwbuf_size);
>
> Ouch ! That's a high data rate memcpy that can be avoided. Why don't you
> DMA directly to the vb2 buffers ? You will need to use videobuf2-dma-
> contig instead of videobuf2-vmalloc, but apart from that there should be
> no issue.
Yes. We thought about this issue and considered this approach as a trade-off to avoid DRIF overflow issue. Overflow happens DMAC fails to consume from DRIF FIFO on time (i.e.) when user app is busy processing samples and fails to queue buffers to DMAC. After an overflow, the device needs to reset (streaming stop/restart sequence). With cyclic DMA, we de-coupled the h/w and user buffers to avoid a costly device reset when user app is busy or not scheduled. Some samples will be lost but that is identified with the sequence numbers and the action/policy can be left to the user. Are you OK with this please?
>
> > + if ((bufi->status | bufq->status) & RCAR_DRIF_BUF_OVERFLOW) {
> > + overflow = true;
> > + /* Clear the flag in status */
> > + bufi->status &= ~RCAR_DRIF_BUF_OVERFLOW;
> > + bufq->status &= ~RCAR_DRIF_BUF_OVERFLOW;
> > + }
> > + } else {
> > + struct rcar_drif_hwbuf *bufiq;
> > +
> > + /* Get fbuf */
> > + fbuf = rcar_drif_get_fbuf(sdr);
> > + if (!fbuf)
> > + return;
> > +
> > + bufiq = ch->buf[idx];
> > +
> > + memcpy(vb2_plane_vaddr(&fbuf->vb.vb2_buf, 0),
> > + bufiq->addr, sdr->hwbuf_size);
> > +
> > + if (bufiq->status & RCAR_DRIF_BUF_OVERFLOW) {
> > + overflow = true;
> > + /* Clear the flag in status */
> > + bufiq->status &= ~RCAR_DRIF_BUF_OVERFLOW;
> > + }
> > + }
> > +
> > + rdrif_dbg(2, sdr, "ch%u: prod %u\n", ch->num, sdr->produced);
> > +
> > + fbuf->vb.field = V4L2_FIELD_NONE;
> > + fbuf->vb.sequence = sdr->produced++;
> > + fbuf->vb.vb2_buf.timestamp = ktime_get_ns();
> > + vb2_set_plane_payload(&fbuf->vb.vb2_buf, 0,
> > + formats[sdr->fmt_idx].buffersize);
> > +
> > + /* Set error state on overflow */
> > + if (overflow)
> > + vb2_buffer_done(&fbuf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
> > + else
> > + vb2_buffer_done(&fbuf->vb.vb2_buf, VB2_BUF_STATE_DONE);
>
> Maybe
>
> vb2_buffer_done(&fbuf->vb.vb2_buf,
> overflow ? VB2_BUF_STATE_ERROR: VB2_BUF_STATE_DONE);
Agreed
>
> > +}
> > +
> > +/* DMA callback for each stage */
> > +static void rcar_drif_dma_complete(void *dma_async_param) {
> > + struct rcar_drif *ch = dma_async_param;
> > + struct rcar_drif_sdr *sdr = ch->sdr;
> > +
> > + mutex_lock(&sdr->vb_queue_mutex);
>
> Isn't the complete callback potentially called in interrupt context ? I
> know the rcar-dmac driver uses a threaded interrupt handler for that, but
> is that a guarantee of the DMA engine API ?
>
DMA engine API mentions the callback will be called in tasklet context. Hmm... I could convert that into spin_lock_irq() if that's OK.
> > +
> > + /* DMA can be terminated while the callback was waiting on lock */
> > + if (!vb2_is_streaming(&sdr->vb_queue))
>
> Can it ? The streaming flag is cleared after the stop_streaming operation
> is called, which will terminate all DMA transfers synchronously.
rcar-dmac did not have device_synchronize support, when I started with this patch set. There is a comment in the terminal_all call
1224 /*
1225 * FIXME: No new interrupt can occur now, but the IRQ thread might still
1226 * be running.
1227 */
Hence, I was a bit paranoid.
>
> > + goto stopped;
> > +
> > + rcar_drif_deliver_buf(ch);
> > +stopped:
> > + mutex_unlock(&sdr->vb_queue_mutex);
> > +}
> > +
> > +static int rcar_drif_qbuf(struct rcar_drif *ch) {
> > + struct rcar_drif_sdr *sdr = ch->sdr;
> > + dma_addr_t addr = ch->dma_handle;
> > + struct dma_async_tx_descriptor *rxd;
> > + dma_cookie_t cookie;
> > + int ret = -EIO;
> > +
> > + /* Setup cyclic DMA with given buffers */
> > + rxd = dmaengine_prep_dma_cyclic(ch->dmach, addr,
> > + sdr->hwbuf_size * num_hwbufs,
> > + sdr->hwbuf_size, DMA_DEV_TO_MEM,
> > + DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> > + if (!rxd) {
> > + rdrif_err(sdr, "ch%u: prep dma cyclic failed\n", ch->num);
> > + return ret;
> > + }
> > +
> > + /* Submit descriptor */
> > + rxd->callback = rcar_drif_dma_complete;
> > + rxd->callback_param = ch;
> > + cookie = dmaengine_submit(rxd);
> > + if (dma_submit_error(cookie)) {
> > + rdrif_err(sdr, "ch%u: dma submit failed\n", ch->num);
> > + return ret;
> > + }
> > +
> > + dma_async_issue_pending(ch->dmach);
> > + return 0;
> > +}
> > +
> > +/* Enable reception */
> > +static int rcar_drif_enable_rx(struct rcar_drif_sdr *sdr) {
> > + unsigned int i;
> > + u32 ctr;
> > + int ret;
> > +
> > + /*
> > + * When both internal channels are enabled, they can be synchronized
> > + * only by the master
> > + */
> > +
> > + /* Enable receive */
> > + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> > + ctr = readl(sdr->ch[i]->base + RCAR_DRIF_SICTR);
> > + ctr |= (RCAR_DRIF_SICTR_RX_RISING_EDGE |
> > + RCAR_DRIF_SICTR_RX_EN);
> > + writel(ctr, sdr->ch[i]->base + RCAR_DRIF_SICTR);
> > + }
> > +
> > + /* Check receive enabled */
> > + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> > + ret = readl_poll_timeout(sdr->ch[i]->base + RCAR_DRIF_SICTR,
> > + ctr, ctr & RCAR_DRIF_SICTR_RX_EN,
> > + 2, 500000);
>
> A 2µs sleep for a 500ms total timeout seems very low to me, that will
> stress the CPU. Same comment for the other locations where you use
> readl_poll_timeout.
>
> How long does the channel typically take to get enabled ?
It takes ~6-7µs on my board. The manual did not specify the expected time and I used a worst case of 500ms as this is used in the on/off path only.
Would 7µs sleep time be acceptable instead of 2µs?
>
> > + if (ret) {
> > + rdrif_err(sdr, "ch%u: rx en failed. ctr 0x%08x\n",
> > + i, readl(sdr->ch[i]->base +
> RCAR_DRIF_SICTR));
> > + break;
> > + }
> > + }
> > + return ret;
> > +}
> > +
> > +/* Disable reception */
> > +static void rcar_drif_disable_rx(struct rcar_drif_sdr *sdr) {
> > + unsigned int i;
> > + u32 ctr;
> > + int ret;
> > +
> > + /* Disable receive */
> > + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> > + ctr = readl(sdr->ch[i]->base + RCAR_DRIF_SICTR);
> > + ctr &= ~RCAR_DRIF_SICTR_RX_EN;
> > + writel(ctr, sdr->ch[i]->base + RCAR_DRIF_SICTR);
> > + }
> > +
> > + /* Check receive disabled */
> > + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> > + ret = readl_poll_timeout(sdr->ch[i]->base + RCAR_DRIF_SICTR,
> > + ctr, !(ctr & RCAR_DRIF_SICTR_RX_EN),
> > + 2, 500000);
>
> How long does the channel typically take to get disabled ?
Same as above comment.
>
> > + if (ret)
> > + dev_warn(&sdr->vdev->dev,
> > + "ch%u: failed to disable rx. ctr 0x%08x\n",
> > + i, readl(sdr->ch[i]->base + RCAR_DRIF_SICTR));
> > + }
> > +}
> > +
> > +/* Start channel */
> > +static int rcar_drif_start_channel(struct rcar_drif *ch) {
> > + struct rcar_drif_sdr *sdr = ch->sdr;
> > + u32 ctr, str;
> > + int ret;
> > +
> > + /* Reset receive */
> > + writel(RCAR_DRIF_SICTR_RESET, ch->base + RCAR_DRIF_SICTR);
> > + ret = readl_poll_timeout(ch->base + RCAR_DRIF_SICTR,
> > + ctr, !(ctr & RCAR_DRIF_SICTR_RESET),
>
> The alignment is weird.
If I remember correctly, it is a checkpatch warning when I started with the patch set (Last year Oct timeframe).
>
> > + 2, 500000);
> > + if (ret) {
> > + rdrif_err(sdr, "ch%u: failed to reset rx. ctr 0x%08x\n",
> > + ch->num, readl(ch->base + RCAR_DRIF_SICTR));
> > + return ret;
> > + }
> > +
> > + /* Queue buffers for DMA */
> > + ret = rcar_drif_qbuf(ch);
> > + if (ret)
> > + return ret;
> > +
> > + /* Clear status register flags */
> > + str = RCAR_DRIF_RFFUL | RCAR_DRIF_REOF | RCAR_DRIF_RFSERR |
> > + RCAR_DRIF_RFUDF | RCAR_DRIF_RFOVF;
> > + writel(str, ch->base + RCAR_DRIF_SISTR);
> > +
> > + /* Enable DMA receive interrupt */
> > + writel(0x00009000, ch->base + RCAR_DRIF_SIIER);
> > +
> > + return ret;
> > +}
> > +
> > +/* Start receive operation */
> > +static int rcar_drif_start(struct rcar_drif_sdr *sdr) {
> > + unsigned int i;
> > + int ret;
> > +
> > + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> > + ret = rcar_drif_start_channel(sdr->ch[i]);
> > + if (ret)
> > + goto start_error;
> > + }
> > +
> > + sdr->produced = 0;
> > + ret = rcar_drif_enable_rx(sdr);
> > +start_error:
>
> Don't you need to stop the channels that were successfully started if an
> error occurs ?
Thank you. I missed this :-(.
>
> > + return ret;
> > +}
> > +
> > +/* Stop channel */
> > +static void rcar_drif_stop_channel(struct rcar_drif *ch) {
> > + struct rcar_drif_sdr *sdr = ch->sdr;
> > + int ret, retries = 3;
> > +
> > + /* Disable DMA receive interrupt */
> > + writel(0x00000000, ch->base + RCAR_DRIF_SIIER);
> > +
> > + do {
> > + /* Terminate all DMA transfers */
> > + ret = dmaengine_terminate_sync(ch->dmach);
> > + if (!ret)
> > + break;
> > + rdrif_dbg(2, sdr, "stop retry\n");
> > + } while (--retries);
>
> Why do you need to retry the terminate operation, why does it fail ?
Yes, I think it can be removed. I cannot remember why I added retry here because rcar-dmac terminate_all always seem to return 0. Last year, there used to be a WARN_ON in rcar-dmac code when channel halt fails (i.e.) it is not guaranteed that all transfers are stopped even after calling dmaengine_terminate_sync(). With the character version of this driver, I used to have a test app to start/stream/stop/restart sequence & hit the WARN_ON in dmac code sometimes (https://www.spinics.net/lists/linux-renesas-soc/msg04840.html). I may have added this return value check assuming it is passed on but I cannot see that even in the old code.
So yes, I agree with your comment.
>
> > + WARN_ON(!retries);
> > +}
>
> [snip]
>
> > +/* Start streaming */
> > +static int rcar_drif_start_streaming(struct vb2_queue *vq, unsigned
> > +int
> > count)
> > +{
> > + struct rcar_drif_sdr *sdr = vb2_get_drv_priv(vq);
> > + unsigned int i, j;
> > + int ret;
> > +
> > + mutex_lock(&sdr->v4l2_mutex);
>
> I'm surprised, aren't the start_streaming and stop_streaming operations
> called with the video device lock held already by the v4l2-ioctl layer ? I
> think they should be, if they're not there's probably a bug somewhere.
I did not see it. Please correct me if this is wrong
v4l_streamon
vb2_ioctl_streamon
vb2_streamon
vb2_core_streamon
vb2_start_streaming
q->ops->start_streaming => rcar_drif_start_streaming
>
> > + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> > + ret = clk_prepare_enable(sdr->ch[i]->clkp);
> > + if (ret)
> > + goto start_error;
> > + }
> > +
> > + /* Set default MDRx settings */
> > + rcar_drif_set_mdr1(sdr);
> > +
> > + /* Set new format */
> > + ret = rcar_drif_set_format(sdr);
> > + if (ret)
> > + goto start_error;
> > +
> > + if (sdr->num_cur_ch == RCAR_DRIF_MAX_CHANNEL)
> > + sdr->hwbuf_size =
> > + formats[sdr->fmt_idx].buffersize / RCAR_DRIF_MAX_CHANNEL;
> > + else
> > + sdr->hwbuf_size = formats[sdr->fmt_idx].buffersize;
> > +
> > + rdrif_dbg(1, sdr, "num_hwbufs %u, hwbuf_size %u\n",
> > + num_hwbufs, sdr->hwbuf_size);
> > +
> > + /* Alloc DMA channel */
> > + ret = rcar_drif_alloc_dmachannel(sdr);
> > + if (ret)
> > + goto start_error;
> > +
> > + /* Alloc buf context */
> > + ret = rcar_drif_alloc_bufctxt(sdr);
> > + if (ret)
> > + goto start_error;
> > +
> > + /* Request buffers */
> > + ret = rcar_drif_request_buf(sdr);
> > + if (ret)
> > + goto start_error;
> > +
> > + /* Start Rx */
> > + ret = rcar_drif_start(sdr);
> > + if (ret)
> > + goto start_error;
> > +
> > + mutex_unlock(&sdr->v4l2_mutex);
> > + rdrif_dbg(1, sdr, "started\n");
> > + return ret;
> > +
> > +start_error:
>
> As there's a single error label I would call this "error". Up to you.
Agreed.
>
> > + rcar_drif_release_queued_bufs(sdr, VB2_BUF_STATE_QUEUED);
> > + rcar_drif_release_buf(sdr);
> > + rcar_drif_release_bufctxt(sdr);
> > + rcar_drif_release_dmachannel(sdr);
> > + for (j = 0; j < i; j++)
> > + clk_disable_unprepare(sdr->ch[j]->clkp);
> > +
> > + mutex_unlock(&sdr->v4l2_mutex);
> > + return ret;
> > +}
>
> [snip]
>
> > +/* Vb2 ops */
> > +static struct vb2_ops rcar_drif_vb2_ops = {
>
> You can make this static const.
Agreed
>
> > + .queue_setup = rcar_drif_queue_setup,
> > + .buf_queue = rcar_drif_buf_queue,
> > + .start_streaming = rcar_drif_start_streaming,
> > + .stop_streaming = rcar_drif_stop_streaming,
> > + .wait_prepare = vb2_ops_wait_prepare,
> > + .wait_finish = vb2_ops_wait_finish,
> > +};
>
> [snip]
>
> > +static int rcar_drif_g_fmt_sdr_cap(struct file *file, void *priv,
> > + struct v4l2_format *f)
> > +{
> > + struct rcar_drif_sdr *sdr = video_drvdata(file);
> > +
> > + f->fmt.sdr.pixelformat = formats[sdr->fmt_idx].pixelformat;
> > + f->fmt.sdr.buffersize = formats[sdr->fmt_idx].buffersize;
> > + memset(f->fmt.sdr.reserved, 0, sizeof(f->fmt.sdr.reserved));
>
> I believe the core ioctl handling code already does this for you. Same for
> the other ioctl handlers in
Agreed
>
> > + return 0;
> > +}
> > +
> > +static int rcar_drif_s_fmt_sdr_cap(struct file *file, void *priv,
> > + struct v4l2_format *f)
> > +{
> > + struct rcar_drif_sdr *sdr = video_drvdata(file);
> > + struct vb2_queue *q = &sdr->vb_queue;
> > + unsigned int i;
> > +
> > + if (vb2_is_busy(q))
> > + return -EBUSY;
> > +
> > + memset(f->fmt.sdr.reserved, 0, sizeof(f->fmt.sdr.reserved));
> > + for (i = 0; i < NUM_FORMATS; i++) {
> > + if (formats[i].pixelformat == f->fmt.sdr.pixelformat) {
>
> The code would become more readable (at least in my opinion) if you just
> added a break here, and moved the code below after the loop. In case the
> requested format isn't found (i == NUM_FORMATS) you can then set i to 0
> and proceed, that will select the first available format as a default.
Agreed
>
> > + sdr->fmt_idx = i;
> > + f->fmt.sdr.buffersize = formats[i].buffersize;
> > +
> > + /*
> > + * If a format demands one channel only out of two
> > + * enabled channels, pick the 0th channel.
> > + */
> > + if (formats[i].num_ch < sdr->num_hw_ch) {
> > + sdr->cur_ch_mask = BIT(0);
> > + sdr->num_cur_ch = formats[i].num_ch;
> > + } else {
> > + sdr->cur_ch_mask = sdr->hw_ch_mask;
> > + sdr->num_cur_ch = sdr->num_hw_ch;
> > + }
> > +
> > + rdrif_dbg(1, sdr, "cur: idx %u mask %lu num %u\n",
> > + i, sdr->cur_ch_mask, sdr->num_cur_ch);
> > + return 0;
> > + }
> > + }
> > +
> > + if (rcar_drif_set_default_format(sdr)) {
> > + rdrif_err(sdr, "cannot set default format\n");
> > + return -EINVAL;
> > + }
> > +
> > + f->fmt.sdr.pixelformat = formats[sdr->fmt_idx].pixelformat;
> > + f->fmt.sdr.buffersize = formats[sdr->fmt_idx].buffersize;
> > + return 0;
> > +}
> > +
> > +static int rcar_drif_try_fmt_sdr_cap(struct file *file, void *priv,
> > + struct v4l2_format *f)
> > +{
> > + struct rcar_drif_sdr *sdr = video_drvdata(file);
> > + unsigned int i;
> > +
> > + memset(f->fmt.sdr.reserved, 0, sizeof(f->fmt.sdr.reserved));
> > + for (i = 0; i < NUM_FORMATS; i++) {
> > + if (formats[i].pixelformat == f->fmt.sdr.pixelformat) {
> > + f->fmt.sdr.buffersize = formats[i].buffersize;
> > + return 0;
> > + }
> > + }
> > +
> > + f->fmt.sdr.pixelformat = formats[sdr->fmt_idx].pixelformat;
> > + f->fmt.sdr.buffersize = formats[sdr->fmt_idx].buffersize;
>
> The result of the TRY_FMT ioctl should not depend on the currently
> configured format. I would return a fixed format (for instance the first
> one in the formats array) in the default case.
Agreed
>
> > + return 0;
> > +}
> > +
> > +/* Tuner subdev ioctls */
> > +static int rcar_drif_enum_freq_bands(struct file *file, void *priv,
> > + struct v4l2_frequency_band *band) {
> > + struct rcar_drif_sdr *sdr = video_drvdata(file);
> > + struct v4l2_subdev *sd;
> > + int ret = 0;
> > +
> > + v4l2_device_for_each_subdev(sd, &sdr->v4l2_dev) {
> > + ret = v4l2_subdev_call(sd, tuner, enum_freq_bands, band);
>
> This won't work as-is when you'll have multiple subdevs. As the driver
> only supports a single connected subdev at the moment, I suggest storing a
> pointer to that subdev in the rcar_drif_sdr structure, and calling
> operations on that subdev explicitly instead of looping over all subdevs.
> The comment holds for all other ioctls.
Agreed.
>
> > + if (ret)
> > + break;
> > + }
> > + return ret;
> > +}
>
> [snip]
>
> > +static int rcar_drif_notify_bound(struct v4l2_async_notifier *notifier,
> > + struct v4l2_subdev *subdev,
> > + struct v4l2_async_subdev *asd) {
> > + struct rcar_drif_sdr *sdr =
> > + container_of(notifier, struct rcar_drif_sdr, notifier);
> > +
> > + /* Nothing to do at this point */
>
> If there's nothing to do you can just leave the bound callback
> unimplemented, it's optional.
Agreed.
>
> > + rdrif_dbg(2, sdr, "bound asd: %s\n", asd->match.of.node->name);
> > + return 0;
> > +}
> > +
> > +/* Sub-device registered notification callback */ static int
> > +rcar_drif_notify_complete(struct v4l2_async_notifier *notifier) {
> > + struct rcar_drif_sdr *sdr =
> > + container_of(notifier, struct rcar_drif_sdr, notifier);
> > + struct v4l2_subdev *sd;
> > + int ret;
> > +
> > + sdr->v4l2_dev.ctrl_handler = &sdr->ctrl_hdl;
> > +
> > + ret = v4l2_device_register_subdev_nodes(&sdr->v4l2_dev);
> > + if (ret) {
> > + rdrif_err(sdr, "failed register subdev nodes ret %d\n", ret);
> > + return ret;
> > + }
>
> Do you need to expose subdev nodes to userspace ? Can't everything be
> handled from the V4L2 SDR node ?
As of today, everything can be handled from the V4L2 SDR node with current MAX2175 subdev. If the tuner driver is enhanced later, this would help.
>
> > + v4l2_device_for_each_subdev(sd, &sdr->v4l2_dev) {
> > + ret = v4l2_ctrl_add_handler(sdr->v4l2_dev.ctrl_handler,
> > + sd->ctrl_handler, NULL);
>
> Shouldn't you undo this somewhere when unbinding the subdevs ?
>
> > + if (ret) {
> > + rdrif_err(sdr, "failed ctrl add hdlr ret %d\n", ret);
> > + return ret;
> > + }
> > + }
> > + rdrif_dbg(2, sdr, "notify complete\n");
> > + return 0;
> > +}
>
> [snip]
>
> > +/* Parse sub-devs (tuner) to find a matching device */ static int
> > +rcar_drif_parse_subdevs(struct rcar_drif_sdr *sdr,
> > + struct device *dev)
> > +{
> > + struct v4l2_async_notifier *notifier = &sdr->notifier;
> > + struct rcar_drif_async_subdev *rsd;
> > + struct device_node *node;
> > +
> > + notifier->subdevs = devm_kzalloc(dev, sizeof(*notifier->subdevs),
> > + GFP_KERNEL);
> > + if (!notifier->subdevs)
> > + return -ENOMEM;
> > +
> > + node = of_graph_get_next_endpoint(dev->of_node, NULL);
> > + if (!node)
> > + return 0;
> > +
> > + rsd = devm_kzalloc(dev, sizeof(*rsd), GFP_KERNEL);
> > + if (!rsd) {
> > + of_node_put(node);
>
> If you move the allocation above of_graph_get_next_endpoint() you won't
> have to call of_node_put() in the error path.
Agreed
>
> > + return -ENOMEM;
> > + }
> > +
> > + notifier->subdevs[notifier->num_subdevs] = &rsd->asd;
> > + rsd->asd.match.of.node = of_graph_get_remote_port_parent(node);
>
> Aren't you missing an of_node_put() on the returned node ? Or does the
> async framework take care of that ?
You are right. of_node_put() on the returned node is missing. I will add it.
>
> > + of_node_put(node);
> > + if (!rsd->asd.match.of.node) {
> > + dev_warn(dev, "bad remote port parent\n");
> > + return -EINVAL;
> > + }
> > +
> > + rsd->asd.match_type = V4L2_ASYNC_MATCH_OF;
> > + notifier->num_subdevs++;
> > +
> > + /* Get the endpoint properties */
> > + rcar_drif_get_ep_properties(sdr, node);
> > + return 0;
> > +}
> > +
> > +/* Check if the given device is the primary bond */ static bool
> > +rcar_drif_primary_bond(struct platform_device *pdev) {
> > + if (of_find_property(pdev->dev.of_node, "renesas,primary-bond",
> NULL))
> > + return true;
> > +
> > + return false;
>
> How about
>
> return of_property_read_bool(pdev->dev.of_node,
> "renesas,primary-bond");
Shall I remove this function itself and just use the property in the "if" condition please? It's used in one place only. I tend to not touch the bindings name/type as we have some sort of agreement after ~4months time :-)
>
> > +}
> > +
> > +/* Get the bonded platform dev if enabled */ static struct
> > +platform_device *rcar_drif_enabled_bond(struct
> > platform_device *p)
> > +{
> > + struct device_node *np;
> > +
> > + np = of_parse_phandle(p->dev.of_node, "renesas,bonding", 0);
>
> The function takes a reference to np, you need to call of_node_put() on it
> (only if the returned pointer isn't NULL).
Agreed
>
> > + if (np && of_device_is_available(np))
> > + return of_find_device_by_node(np);
>
> of_find_device_by_node() takes a reference to the returned device, you
> need to call device_put() on it when you don't need it anymore.
Agreed & Thanks again.
>
>
> > + return NULL;
> > +}
> > +
> > +/* Proble internal channel */
> > +static int rcar_drif_channel_probe(struct platform_device *pdev)
> > +{
> > + struct rcar_drif *ch;
> > + struct resource *res;
> > + void __iomem *base;
> > + struct clk *clkp;
>
> Maybe s/clkp/clk/ ?
Agreed
>
> > + int ret;
> > +
> > + /* Peripheral clock */
> > + clkp = devm_clk_get(&pdev->dev, "fck");
> > + if (IS_ERR(clkp)) {
> > + ret = PTR_ERR(clkp);
> > + dev_err(&pdev->dev, "clk get failed (%d)\n", ret);
> > + return ret;
> > + }
>
> Isn't the clock managed automatically by runtime PM ?
I think the driver need to support runtime PM in order to manage the clock. Otherwise it just gets adds the clk to genpd (_prepare) without enable/disable. I need to double check this.
>
> > + /* Register map */
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + base = devm_ioremap_resource(&pdev->dev, res);
> > + if (IS_ERR(base)) {
> > + ret = PTR_ERR(base);
> > + dev_err(&pdev->dev, "ioremap failed (%d)\n", ret);
> > + return ret;
>
> devm_ioremap_resource() already prints an error message, you can remove
> this
> one.
Agreed
>
> > + }
> > +
> > + /* Reserve memory for enabled channel */
> > + ch = devm_kzalloc(&pdev->dev, sizeof(*ch), GFP_KERNEL);
> > + if (!ch) {
> > + ret = PTR_ERR(ch);
> > + dev_err(&pdev->dev, "failed alloc channel\n");
>
> Memory allocation failures already print error messages, you can remove
> this
> one.
Agreed.
>
> > + return ret;
> > + }
> > + ch->pdev = pdev;
> > + ch->clkp = clkp;
> > + ch->base = base;
> > + ch->start = res->start;
>
> If you allocated the ch structure first you could set the fields directly
> without a need for local variables.
Agreed.
>
> > + platform_set_drvdata(pdev, ch);
> > + return 0;
> > +}
> > +
> > +static int rcar_drif_probe(struct platform_device *pdev)
> > +{
> > + struct rcar_drif *ch, *b_ch = NULL;
> > + struct platform_device *b_pdev;
> > + struct rcar_drif_sdr *sdr;
> > + int ret;
> > +
> > + /* Probe internal channel */
> > + ret = rcar_drif_channel_probe(pdev);
> > + if (ret)
> > + return ret;
>
> I would have done it the other way around, inlining the
> rcar_drif_channel_probe() function here as that's the common case, and
> moving
> the V4L2 SDR device initialization code to a different function.
Agreed
>
> > + /* Check if both channels of the bond are enabled */
> > + b_pdev = rcar_drif_enabled_bond(pdev);
> > + if (b_pdev) {
> > + /* Check if current channel acting as primary-bond */
> > + if (!rcar_drif_primary_bond(pdev)) {
> > + dev_notice(&pdev->dev, "probed\n");
> > + return 0;
> > + }
> > +
> > + /* Check if the other device is probed */
> > + b_ch = platform_get_drvdata(b_pdev);
> > + if (!b_ch) {
> > + dev_info(&pdev->dev, "defer probe\n");
> > + return -EPROBE_DEFER;
> > + }
>
> Isn't this all very racy ? What if the other channel's device is removed
> while
> this one is probed ?
OK. Will holding the device_lock(&b_pdev->dev) is sufficient?
>
> > + /* Set the other channel number */
> > + b_ch->num = 1;
>
> Reading data from the other channel's private structure is one thing, but
> writing it makes me shiver :-S Could we make it so that 0 is the slave and
> 1
> the master ? That way you would set ch->num = 1 instead of b_ch->num = 1,
> keeping all modifications to the private structure local to the device
> being
> probed.
Agreed.
>
> > + }
> > +
> > + /* Channel acting as SDR instance */
> > + ch = platform_get_drvdata(pdev);
> > + ch->acting_sdr = true;
> > +
> > + /* Reserve memory for SDR structure */
> > + sdr = devm_kzalloc(&pdev->dev, sizeof(*sdr), GFP_KERNEL);
> > + if (!sdr) {
> > + ret = PTR_ERR(sdr);
> > + dev_err(&pdev->dev, "failed alloc drif context\n");
> > + return ret;
> > + }
> > + sdr->dev = &pdev->dev;
> > + sdr->hw_ch_mask = BIT(ch->num);
> > +
> > + /* Establish links between SDR and channel(s) */
> > + ch->sdr = sdr;
> > + sdr->ch[ch->num] = ch;
> > + if (b_ch) {
> > + sdr->ch[b_ch->num] = b_ch;
> > + b_ch->sdr = sdr;
> > + sdr->hw_ch_mask |= BIT(b_ch->num);
> > + }
> > + sdr->num_hw_ch = hweight_long(sdr->hw_ch_mask);
> > +
> > + /* Validate any supported format for enabled channels */
> > + ret = rcar_drif_set_default_format(sdr);
> > + if (ret) {
> > + dev_err(sdr->dev, "failed to set default format\n");
> > + return ret;
> > + }
> > +
> > + /* Set defaults */
> > + sdr->hwbuf_size = RCAR_DRIF_DEFAULT_HWBUF_SIZE;
> > +
> > + mutex_init(&sdr->v4l2_mutex);
> > + mutex_init(&sdr->vb_queue_mutex);
> > + spin_lock_init(&sdr->queued_bufs_lock);
> > + INIT_LIST_HEAD(&sdr->queued_bufs);
> > +
> > + /* Init videobuf2 queue structure */
> > + sdr->vb_queue.type = V4L2_BUF_TYPE_SDR_CAPTURE;
> > + sdr->vb_queue.io_modes = VB2_READ | VB2_MMAP | VB2_DMABUF;
> > + sdr->vb_queue.drv_priv = sdr;
> > + sdr->vb_queue.buf_struct_size = sizeof(struct rcar_drif_frame_buf);
> > + sdr->vb_queue.ops = &rcar_drif_vb2_ops;
> > + sdr->vb_queue.mem_ops = &vb2_vmalloc_memops;
> > + sdr->vb_queue.timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> > +
> > + /* Init videobuf2 queue */
> > + ret = vb2_queue_init(&sdr->vb_queue);
> > + if (ret) {
> > + dev_err(sdr->dev, "could not initialize vb2 queue\n");
> > + return ret;
> > + }
> > +
> > + /* Register the v4l2_device */
> > + ret = v4l2_device_register(&pdev->dev, &sdr->v4l2_dev);
> > + if (ret) {
> > + dev_err(sdr->dev, "failed v4l2_device_register (%d)\n", ret);
>
> Maybe "failed to register V4L2 device" to make it a real sentence ? :-)
Agreed
>
> > + return ret;
> > + }
> > +
> > + /*
> > + * Parse subdevs after v4l2_device_register because if the subdev
> > + * is already probed, bound and complete will be called immediately
> > + */
> > + ret = rcar_drif_parse_subdevs(sdr, &pdev->dev);
> > + if (ret)
> > + goto err_unreg_v4l2;
> > +
> > + sdr->notifier.bound = rcar_drif_notify_bound;
> > + sdr->notifier.complete = rcar_drif_notify_complete;
> > +
> > + v4l2_ctrl_handler_init(&sdr->ctrl_hdl, 10);
>
> Possibly a stupid question, why 10, if you don't create any control in
> this
> driver ?
To accommodate the subdev controls.
>
> > + /* Register notifier */
> > + ret = v4l2_async_notifier_register(&sdr->v4l2_dev, &sdr->notifier);
> > + if (ret < 0) {
> > + dev_err(sdr->dev, "notifier registration failed (%d)\n", ret);
> > + goto err_free_ctrls;
> > + }
> > +
> > + /* Init video_device structure */
> > + sdr->vdev = video_device_alloc();
> > + if (!sdr->vdev) {
> > + ret = -ENOMEM;
> > + goto err_unreg_notif;
> > + }
> > + snprintf(sdr->vdev->name, sizeof(sdr->vdev->name), "R-Car DRIF");
> > + sdr->vdev->fops = &rcar_drif_fops;
> > + sdr->vdev->ioctl_ops = &rcar_drif_ioctl_ops;
> > + sdr->vdev->release = video_device_release;
> > + sdr->vdev->lock = &sdr->v4l2_mutex;
> > + sdr->vdev->queue = &sdr->vb_queue;
> > + sdr->vdev->queue->lock = &sdr->vb_queue_mutex;
> > + sdr->vdev->ctrl_handler = &sdr->ctrl_hdl;
> > + sdr->vdev->v4l2_dev = &sdr->v4l2_dev;
> > + sdr->vdev->device_caps = V4L2_CAP_SDR_CAPTURE | V4L2_CAP_TUNER |
> > + V4L2_CAP_STREAMING | V4L2_CAP_READWRITE;
> > + video_set_drvdata(sdr->vdev, sdr);
> > +
> > + /* Register V4L2 SDR device */
> > + ret = video_register_device(sdr->vdev, VFL_TYPE_SDR, -1);
> > + if (ret) {
> > + dev_err(sdr->dev, "failed video_register_device (%d)\n", ret);
>
> Same here, "failed to register video device" ?
Agreed.
>
> > + goto err_unreg_notif;
> > + }
> > +
> > + dev_notice(sdr->dev, "probed\n");
>
> Do you think this message is really useful ? I believe it would just add a
> bit
> more noise to the kernel log, without any real use.
OK. Will remove it.
>
> > + return 0;
> > +
> > +err_unreg_notif:
> > + video_device_release(sdr->vdev);
> > + v4l2_async_notifier_unregister(&sdr->notifier);
> > +err_free_ctrls:
> > + v4l2_ctrl_handler_free(&sdr->ctrl_hdl);
> > +err_unreg_v4l2:
> > + v4l2_device_unregister(&sdr->v4l2_dev);
> > + return ret;
> > +}
> > +
> > +static int rcar_drif_remove(struct platform_device *pdev)
> > +{
> > + struct rcar_drif *ch = platform_get_drvdata(pdev);
> > + struct rcar_drif_sdr *sdr = ch->sdr;
> > +
> > + if (!ch->acting_sdr) {
>
> Isn't it possible to check the channel number instead and remove the
> acting_sdr field ?
Agreed.
>
> > + /* Nothing to do */
> > + dev_notice(&pdev->dev, "removed\n");
> > + return 0;
> > + }
> > +
> > + /* SDR instance */
> > + v4l2_ctrl_handler_free(sdr->vdev->ctrl_handler);
> > + v4l2_async_notifier_unregister(&sdr->notifier);
> > + v4l2_device_unregister(&sdr->v4l2_dev);
> > + video_unregister_device(sdr->vdev);
> > + dev_notice(&pdev->dev, "removed\n");
>
> Even more than the probed message, I think this one can go away.
Agreed.
>
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused rcar_drif_suspend(struct device *dev)
> > +{
> > + return 0;
>
> Maybe a /* FIXME: Implement suspend/resume support */ ?
Agreed.
>
> > +}
> > +
> > +static int __maybe_unused rcar_drif_resume(struct device *dev)
> > +{
> > + return 0;
>
> Same here ?
Agreed.
Thanks,
Ramesh
^ permalink raw reply
* Re: [PATCH] of: introduce event tracepoints for dynamic device_node lifecyle
From: Rob Herring @ 2017-04-18 16:46 UTC (permalink / raw)
To: Tyrel Datwyler
Cc: linuxppc-dev,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Nathan Fontenot, Michael Ellerman, Frank Rowand,
rostedt-nx8X9YLhiw1AfugRpC6u6w, Ingo Molnar
In-Reply-To: <1492475525-10827-1-git-send-email-tyreld-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
On Mon, Apr 17, 2017 at 7:32 PM, Tyrel Datwyler
<tyreld-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> wrote:
> This patch introduces event tracepoints for tracking a device_nodes
> reference cycle as well as reconfig notifications generated in response
> to node/property manipulations.
>
> With the recent upstreaming of the refcount API several device_node
> underflows and leaks have come to my attention in the pseries (DLPAR) dynamic
> logical partitioning code (ie. POWER speak for hotplugging virtual and physcial
> resources at runtime such as cpus or IOAs). These tracepoints provide a
> easy and quick mechanism for validating the reference counting of
> device_nodes during their lifetime.
Not really relevant for this patch, but since you are looking at
pseries and refcounting, the refcounting largely exists for pseries.
It's also hard to get right as this type of fix is fairly common. It's
now used for overlays, but we really probably only need to refcount
the overlays or changesets as a whole, not at a node level. If you
have any thoughts on how a different model of refcounting could work
for pseries, I'd like to discuss it.
> Further, when pseries lpars are migrated to a different machine we
> perform a live update of our device tree to bring it into alignment with the
> configuration of the new machine. The of_reconfig_notify trace point
> provides a mechanism that can be turned for debuging the device tree
> modifications with out having to build a custom kernel to get at the
> DEBUG code introduced by commit 00aa3720.
>
> The following trace events are provided: of_node_get, of_node_put,
> of_node_release, and of_reconfig_notify. These trace points require a kernel
> built with ftrace support to be enabled. In a typical environment where
> debugfs is mounted at /sys/kernel/debug the entire set of tracepoints
> can be set with the following:
>
> echo "of:*" > /sys/kernel/debug/tracing/set_event
>
> or
>
> echo 1 > /sys/kernel/debug/tracing/of/enable
>
> The following shows the trace point data from a DLPAR remove of a cpu
> from a pseries lpar:
>
> cat /sys/kernel/debug/tracing/trace | grep "POWER8@10"
>
> cpuhp/23-147 [023] .... 128.324827:
> of_node_put: refcount=5, dn->full_name=/cpus/PowerPC,POWER8@10
> cpuhp/23-147 [023] .... 128.324829:
> of_node_put: refcount=4, dn->full_name=/cpus/PowerPC,POWER8@10
> cpuhp/23-147 [023] .... 128.324829:
> of_node_put: refcount=3, dn->full_name=/cpus/PowerPC,POWER8@10
> cpuhp/23-147 [023] .... 128.324831:
> of_node_put: refcount=2, dn->full_name=/cpus/PowerPC,POWER8@10
> drmgr-7284 [009] .... 128.439000:
> of_node_put: refcount=1, dn->full_name=/cpus/PowerPC,POWER8@10
> drmgr-7284 [009] .... 128.439002:
> of_reconfig_notify: action=DETACH_NODE, dn->full_name=/cpus/PowerPC,POWER8@10,
> prop->name=null, old_prop->name=null
> drmgr-7284 [009] .... 128.439015:
> of_node_put: refcount=0, dn->full_name=/cpus/PowerPC,POWER8@10
> drmgr-7284 [009] .... 128.439016:
> of_node_release: dn->full_name=/cpus/PowerPC,POWER8@10, dn->_flags=4
>
> Signed-off-by: Tyrel Datwyler <tyreld-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> ---
> drivers/of/dynamic.c | 30 ++++++---------
> include/trace/events/of.h | 93 +++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 105 insertions(+), 18 deletions(-)
> create mode 100644 include/trace/events/of.h
>
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index 888fdbc..85c0966 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -16,6 +16,9 @@
>
> #include "of_private.h"
>
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/of.h>
> +
> /**
> * of_node_get() - Increment refcount of a node
> * @node: Node to inc refcount, NULL is supported to simplify writing of
> @@ -25,8 +28,10 @@
> */
> struct device_node *of_node_get(struct device_node *node)
> {
> - if (node)
> + if (node) {
> kobject_get(&node->kobj);
> + trace_of_node_get(refcount_read(&node->kobj.kref.refcount), node->full_name);
Seems like there should be a kobj wrapper to read the refcount.
> + }
> return node;
> }
> EXPORT_SYMBOL(of_node_get);
> @@ -38,8 +43,10 @@ struct device_node *of_node_get(struct device_node *node)
> */
> void of_node_put(struct device_node *node)
> {
> - if (node)
> + if (node) {
> + trace_of_node_put(refcount_read(&node->kobj.kref.refcount) - 1, node->full_name);
> kobject_put(&node->kobj);
> + }
> }
> EXPORT_SYMBOL(of_node_put);
>
> @@ -92,24 +99,9 @@ int of_reconfig_notifier_unregister(struct notifier_block *nb)
> int of_reconfig_notify(unsigned long action, struct of_reconfig_data *p)
> {
> int rc;
> -#ifdef DEBUG
> - struct of_reconfig_data *pr = p;
>
> - switch (action) {
> - case OF_RECONFIG_ATTACH_NODE:
> - case OF_RECONFIG_DETACH_NODE:
> - pr_debug("notify %-15s %s\n", action_names[action],
> - pr->dn->full_name);
> - break;
> - case OF_RECONFIG_ADD_PROPERTY:
> - case OF_RECONFIG_REMOVE_PROPERTY:
> - case OF_RECONFIG_UPDATE_PROPERTY:
> - pr_debug("notify %-15s %s:%s\n", action_names[action],
> - pr->dn->full_name, pr->prop->name);
> - break;
> + trace_of_reconfig_notify(action, p);
>
> - }
> -#endif
> rc = blocking_notifier_call_chain(&of_reconfig_chain, action, p);
> return notifier_to_errno(rc);
> }
> @@ -326,6 +318,8 @@ void of_node_release(struct kobject *kobj)
> struct device_node *node = kobj_to_device_node(kobj);
> struct property *prop = node->properties;
>
> + trace_of_node_release(node);
> +
> /* We should never be releasing nodes that haven't been detached. */
> if (!of_node_check_flag(node, OF_DETACHED)) {
> pr_err("ERROR: Bad of_node_put() on %s\n", node->full_name);
> diff --git a/include/trace/events/of.h b/include/trace/events/of.h
> new file mode 100644
> index 0000000..0d53271
> --- /dev/null
> +++ b/include/trace/events/of.h
> @@ -0,0 +1,93 @@
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM of
> +
> +#if !defined(_TRACE_OF_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_OF_H
> +
> +#include <linux/of.h>
> +#include <linux/tracepoint.h>
> +
> +DECLARE_EVENT_CLASS(of_node_ref_template,
> +
> + TP_PROTO(int refcount, const char* dn_name),
> +
> + TP_ARGS(refcount, dn_name),
> +
> + TP_STRUCT__entry(
> + __string(dn_name, dn_name)
> + __field(int, refcount)
> + ),
> +
> + TP_fast_assign(
> + __assign_str(dn_name, dn_name);
> + __entry->refcount = refcount;
> + ),
> +
> + TP_printk("refcount=%d, dn->full_name=%s",
> + __entry->refcount, __get_str(dn_name))
> +);
> +
> +DEFINE_EVENT(of_node_ref_template, of_node_get,
> + TP_PROTO(int refcount, const char* dn_name),
> + TP_ARGS(refcount, dn_name));
> +
> +DEFINE_EVENT(of_node_ref_template, of_node_put,
> + TP_PROTO(int refcount, const char* dn_name),
> + TP_ARGS(refcount, dn_name));
> +
> +TRACE_EVENT(of_node_release,
> +
> + TP_PROTO(struct device_node *dn),
> +
> + TP_ARGS(dn),
> +
> + TP_STRUCT__entry(
> + __string(dn_name, dn->full_name)
> + __field(unsigned long, flags)
> + ),
> +
> + TP_fast_assign(
> + __assign_str(dn_name, dn->full_name);
> + __entry->flags = dn->_flags;
> + ),
> +
> + TP_printk("dn->full_name=%s, dn->_flags=%lu",
> + __get_str(dn_name), __entry->flags)
> +);
> +
> +#define of_reconfig_action_names \
> + {OF_RECONFIG_ATTACH_NODE, "ATTACH_NODE"}, \
> + {OF_RECONFIG_DETACH_NODE, "DETACH_NODE"}, \
> + {OF_RECONFIG_ADD_PROPERTY, "ADD_PROPERTY"}, \
> + {OF_RECONFIG_REMOVE_PROPERTY, "REMOVE_PROPERTY"}, \
> + {OF_RECONFIG_UPDATE_PROPERTY, "UPDATE_PROPERTY"}
> +
> +TRACE_EVENT(of_reconfig_notify,
> +
> + TP_PROTO(unsigned long action, struct of_reconfig_data *ord),
> +
> + TP_ARGS(action, ord),
> +
> + TP_STRUCT__entry(
> + __field(unsigned long, action)
> + __string(dn_name, ord->dn->full_name)
> + __string(prop_name, ord->prop ? ord->prop->name : "null")
> + __string(oldprop_name, ord->old_prop ? ord->old_prop->name : "null")
> + ),
> +
> + TP_fast_assign(
> + __entry->action = action;
> + __assign_str(dn_name, ord->dn->full_name);
> + __assign_str(prop_name, ord->prop ? ord->prop->name : "null");
> + __assign_str(oldprop_name, ord->old_prop ? ord->old_prop->name : "null");
> + ),
> +
> + TP_printk("action=%s, dn->full_name=%s, prop->name=%s, old_prop->name=%s",
> + __print_symbolic(__entry->action, of_reconfig_action_names),
> + __get_str(dn_name), __get_str(prop_name), __get_str(oldprop_name))
> +);
> +
> +#endif /* _TRACE_OF_H */
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>
> --
> 1.8.3.1
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] of: fix "/cpus" reference leak in of_numa_parse_cpu_nodes()
From: Rob Herring @ 2017-04-18 16:08 UTC (permalink / raw)
To: David Daney, Tyrel Datwyler
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Frank Rowand,
David Daney, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <1005a5c3-22d1-b082-0383-d8f74e711315-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org>
On Tue, Apr 18, 2017 at 10:16 AM, David Daney <ddaney-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org> wrote:
> On 04/17/2017 05:29 PM, Tyrel Datwyler wrote:
>>
>> The call to of_find_node_by_path("/cpus") returns the cpus device_node
>> with its reference count incremented. There is no matching of_node_put()
>> call in of_numa_parse_cpu_nodes() which results in a leaked reference
>> to the "/cpus" node.
>>
>> This patch adds an of_node_put() to release the reference.
>
>
> Good catch:
> Acked-by: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
Applied, thanks.
Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH V4 1/9] PM / OPP: Allow OPP table to be used for power-domains
From: Sudeep Holla @ 2017-04-18 16:03 UTC (permalink / raw)
To: Viresh Kumar
Cc: Sudeep Holla, Rafael Wysocki, ulf.hansson-QSEj5FYQhm4dnm+yROfE0A,
Kevin Hilman, Viresh Kumar, Nishanth Menon, Stephen Boyd,
linaro-kernel-cunTk1MwBs8s++Sfvej+rw,
linux-pm-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Vincent Guittot,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, lina.iyer-QSEj5FYQhm4dnm+yROfE0A,
rnayak-sgV2jX0FEOL9JmXXK+q4OQ, devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170417053303.GG28191@vireshk-i7>
On 17/04/17 06:33, Viresh Kumar wrote:
> On 13-04-17, 14:43, Sudeep Holla wrote:
>> Interesting. My understand of power domain and in particular power
>> domain performance was that it would control both. The abstract number
>> you introduce would hide clocks and regulators.
>>
>> But if the concept treats it just as yet another regulator, we do we
>> need these at all. Why don't we relate this performance to regulator
>> values and be done with it ?
>>
>> Sorry if I am missing to understand something here. I would look this as
>> replacement for both clocks and regulators, something similar to ACPI
>> CPPC. If not, it looks unnecessary to me with the information I have got
>> so far.
>
> I kind of answered that in the other email.
>
> Some background may be good here. So Qcom tried to solve all this with virtual
> regulators, but the problem was that they need to talk in terms of integer
> values (1, 2, 3..) and not voltages and so they can't use the regulator
> framework straight away. And so we are doing all this.
>
Was it posted externally ? Was there any objections for that approach ?
IMO that's better approach but if I am late to the party, I would like
to read through the discussions that happened on it(if any)
--
Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox