* [PATCH v3 0/3] Update Broadcom Stingray clock entries
From: Ray Jui @ 2018-06-02 0:56 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd, Rob Herring, Mark Rutland
Cc: linux-clk, linux-kernel, bcm-kernel-feedback-list, devicetree,
linux-arm-kernel, Pramod Kumar, Ray Jui
This patch series updates Broadcom Stingray clock entries so they match the
latest ASIC datasheet
This patch series is based off v4.17-rc5 and is available on GIHUB:
repo: https://github.com/Broadcom/arm64-linux.git
branch: sr-clk-v3
Changes since v2:
- Move dt-binding header change to the same patch with the binding doc
update
Changes since v1:
- Fix patch author to Pramod Kumar on all 3 patches
- Fix patch subject spelling error on patch 2/3
Pramod Kumar (3):
dt-bindings: clk: Update Stingray binding doc
clk: bcm: Update and add Stingray clock entries
arm64: dts: Update Stingray clock DT nodes
.../bindings/clock/brcm,iproc-clocks.txt | 26 ++--
.../boot/dts/broadcom/stingray/stingray-clock.dtsi | 26 ++--
drivers/clk/bcm/clk-sr.c | 135 ++++++++++++++++++---
include/dt-bindings/clock/bcm-sr.h | 24 ++--
4 files changed, 170 insertions(+), 41 deletions(-)
--
2.1.4
^ permalink raw reply
* Re: [PATCH 2/3] clk: bcm: Update and add tingray clock entries
From: Ray Jui @ 2018-06-02 0:47 UTC (permalink / raw)
To: Rob Herring
Cc: Michael Turquette, Stephen Boyd, Mark Rutland, linux-clk,
linux-kernel@vger.kernel.org,
maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, devicetree,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Pramod Kumar
In-Reply-To: <CAL_Jsq+YX01CwCycqeu5mxTUiWtnG3HKPJP5mvJO2OHcPF6v6g@mail.gmail.com>
On 6/1/2018 12:02 PM, Rob Herring wrote:
> On Fri, Jun 1, 2018 at 12:56 PM, Ray Jui <ray.jui@broadcom.com> wrote:
>> Hi Rob,
>>
>> On 5/31/2018 9:25 AM, Rob Herring wrote:
>>>
>>> On Fri, May 25, 2018 at 09:45:16AM -0700, Ray Jui wrote:
>>>>
>>>> Update and add Stingray clock definitions and tables so they match the
>>>> binding document and the latest ASIC datasheet
>>>>
>>>> Signed-off-by: Pramod Kumar <pramod.kumar@broadcom.com>
>>>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
>>>> ---
>>>> drivers/clk/bcm/clk-sr.c | 135
>>>> ++++++++++++++++++++++++++++++++-----
>>>> include/dt-bindings/clock/bcm-sr.h | 24 +++++--
>>>
>>>
>>> This goes in the 1st patch.
>>
>>
>> Please help to confirm. You want 1st patch and 2nd patch to be combined into
>> a single patch?
>
> No. include/dt-bindings/* is part of the DT binding, so it goes with
> patch 1. The driver in patch 2.
>
> Rob
>
Okay got it. Thanks!
^ permalink raw reply
* [PATCH v2] of: platform: stop accessing invalid dev in of_platform_device_destroy
From: Srinivas Kandagatla @ 2018-06-02 0:03 UTC (permalink / raw)
To: robh+dt, frowand.list
Cc: linux-arm-msm, bgoswami, devicetree, linux-kernel,
linux-arm-kernel, rohkumar, Srinivas Kandagatla
Immediately after the platform_device_unregister() the device will be cleaned up.
Accessing the freed pointer immediately after that will crash the system.
Found this bug when kernel is built with CONFIG_PAGE_POISONING and testing
loading/unloading audio drivers in a loop on Qcom platforms.
Fix this by removing accessing the dev pointer.
Below is the carsh trace:
Unable to handle kernel paging request at virtual address 6b6b6b6b6b6c03
Mem abort info:
ESR = 0x96000021
Exception class = DABT (current EL), IL = 32 bits
SET = 0, FnV = 0
EA = 0, S1PTW = 0
Data abort info:
ISV = 0, ISS = 0x00000021
CM = 0, WnR = 0
[006b6b6b6b6b6c03] address between user and kernel address ranges
Internal error: Oops: 96000021 [#1] PREEMPT SMP
Modules linked in:
CPU: 2 PID: 1784 Comm: sh Tainted: G W 4.17.0-rc7-02230-ge3a63a7ef641-dirty #204
Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT)
pstate: 80000005 (Nzcv daif -PAN -UAO)
pc : clear_bit+0x18/0x2c
lr : of_platform_device_destroy+0x64/0xb8
sp : ffff00000c9c3930
x29: ffff00000c9c3930 x28: ffff80003d39b200
x27: ffff000008bb1000 x26: 0000000000000040
x25: 0000000000000124 x24: ffff80003a9a3080
x23: 0000000000000060 x22: ffff00000939f518
x21: ffff80003aa79e98 x20: ffff80003aa3dae0
x19: ffff80003aa3c890 x18: ffff800009feb794
x17: 0000000000000000 x16: 0000000000000000
x15: ffff800009feb790 x14: 0000000000000000
x13: ffff80003a058778 x12: ffff80003a058728
x11: ffff80003a058750 x10: 0000000000000000
x9 : 0000000000000006 x8 : ffff80003a825988
x7 : bbbbbbbbbbbbbbbb x6 : 0000000000000001
x5 : 0000000000000000 x4 : 0000000000000001
x3 : 0000000000000008 x2 : 0000000000000001
x1 : 6b6b6b6b6b6b6c03 x0 : 0000000000000000
Process sh (pid: 1784, stack limit = 0x (ptrval))
Call trace:
clear_bit+0x18/0x2c
q6afe_remove+0x20/0x38
apr_device_remove+0x30/0x70
device_release_driver_internal+0x170/0x208
device_release_driver+0x14/0x20
bus_remove_device+0xcc/0x150
device_del+0x10c/0x310
device_unregister+0x1c/0x70
apr_remove_device+0xc/0x18
device_for_each_child+0x50/0x80
apr_remove+0x18/0x20
rpmsg_dev_remove+0x38/0x68
device_release_driver_internal+0x170/0x208
device_release_driver+0x14/0x20
bus_remove_device+0xcc/0x150
device_del+0x10c/0x310
device_unregister+0x1c/0x70
qcom_smd_remove_device+0xc/0x18
device_for_each_child+0x50/0x80
qcom_smd_unregister_edge+0x3c/0x70
smd_subdev_remove+0x18/0x28
rproc_stop+0x48/0xd8
rproc_shutdown+0x60/0xe8
state_store+0xbc/0xf8
dev_attr_store+0x18/0x28
sysfs_kf_write+0x3c/0x50
kernfs_fop_write+0x118/0x1e0
__vfs_write+0x18/0x110
vfs_write+0xa4/0x1a8
ksys_write+0x48/0xb0
sys_write+0xc/0x18
el0_svc_naked+0x30/0x34
Code: d2800022 8b400c21 f9800031 9ac32043 (c85f7c22)
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
Changes since v1:
- fixed issue while reprobing.
drivers/of/platform.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index c00d81dfac0b..84c5c899187b 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -529,10 +529,13 @@ arch_initcall_sync(of_platform_default_populate_init);
int of_platform_device_destroy(struct device *dev, void *data)
{
+ struct device_node *np;
+
/* Do not touch devices not populated from the device tree */
if (!dev->of_node || !of_node_check_flag(dev->of_node, OF_POPULATED))
return 0;
+ np = dev->of_node;
/* Recurse for any nodes that were treated as busses */
if (of_node_check_flag(dev->of_node, OF_POPULATED_BUS))
device_for_each_child(dev, NULL, of_platform_device_destroy);
@@ -544,8 +547,8 @@ int of_platform_device_destroy(struct device *dev, void *data)
amba_device_unregister(to_amba_device(dev));
#endif
- of_node_clear_flag(dev->of_node, OF_POPULATED);
- of_node_clear_flag(dev->of_node, OF_POPULATED_BUS);
+ of_node_clear_flag(np, OF_POPULATED);
+ of_node_clear_flag(np, OF_POPULATED_BUS);
return 0;
}
EXPORT_SYMBOL_GPL(of_platform_device_destroy);
--
2.16.2
^ permalink raw reply related
* Re: [PATCH] of: platform: stop accessing invalid dev in of_platform_device_destroy
From: Srinivas Kandagatla @ 2018-06-01 23:35 UTC (permalink / raw)
To: robh+dt, frowand.list
Cc: linux-arm-msm, bgoswami, devicetree, linux-kernel,
linux-arm-kernel, rohkumar
In-Reply-To: <20180601225822.23439-1-srinivas.kandagatla@linaro.org>
On 01/06/18 23:58, Srinivas Kandagatla wrote:
>
> - of_node_clear_flag(dev->of_node, OF_POPULATED);
> - of_node_clear_flag(dev->of_node, OF_POPULATED_BUS);
This change seems to have a side effect during re-probing. I will dig in
bit more to see how best it can be fixed.
thanks,
srini
^ permalink raw reply
* [PATCH] of: platform: stop accessing invalid dev in of_platform_device_destroy
From: Srinivas Kandagatla @ 2018-06-01 22:58 UTC (permalink / raw)
To: robh+dt, frowand.list
Cc: linux-arm-msm, bgoswami, devicetree, linux-kernel,
linux-arm-kernel, rohkumar, Srinivas Kandagatla
Immediately after the platform_device_unregister() the device will be cleaned up.
Accessing the freed pointer immediately after that will crash the system.
Found this bug when kernel is built with CONFIG_PAGE_POISONING and testing
loading/unloading audio drivers in a loop on Qcom platforms.
Fix this by removing accessing the dev pointer.
Below is the carsh trace:
Unable to handle kernel paging request at virtual address 6b6b6b6b6b6c03
Mem abort info:
ESR = 0x96000021
Exception class = DABT (current EL), IL = 32 bits
SET = 0, FnV = 0
EA = 0, S1PTW = 0
Data abort info:
ISV = 0, ISS = 0x00000021
CM = 0, WnR = 0
[006b6b6b6b6b6c03] address between user and kernel address ranges
Internal error: Oops: 96000021 [#1] PREEMPT SMP
Modules linked in:
CPU: 2 PID: 1784 Comm: sh Tainted: G W 4.17.0-rc7-02230-ge3a63a7ef641-dirty #204
Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT)
pstate: 80000005 (Nzcv daif -PAN -UAO)
pc : clear_bit+0x18/0x2c
lr : of_platform_device_destroy+0x64/0xb8
sp : ffff00000c9c3930
x29: ffff00000c9c3930 x28: ffff80003d39b200
x27: ffff000008bb1000 x26: 0000000000000040
x25: 0000000000000124 x24: ffff80003a9a3080
x23: 0000000000000060 x22: ffff00000939f518
x21: ffff80003aa79e98 x20: ffff80003aa3dae0
x19: ffff80003aa3c890 x18: ffff800009feb794
x17: 0000000000000000 x16: 0000000000000000
x15: ffff800009feb790 x14: 0000000000000000
x13: ffff80003a058778 x12: ffff80003a058728
x11: ffff80003a058750 x10: 0000000000000000
x9 : 0000000000000006 x8 : ffff80003a825988
x7 : bbbbbbbbbbbbbbbb x6 : 0000000000000001
x5 : 0000000000000000 x4 : 0000000000000001
x3 : 0000000000000008 x2 : 0000000000000001
x1 : 6b6b6b6b6b6b6c03 x0 : 0000000000000000
Process sh (pid: 1784, stack limit = 0x (ptrval))
Call trace:
clear_bit+0x18/0x2c
q6afe_remove+0x20/0x38
apr_device_remove+0x30/0x70
device_release_driver_internal+0x170/0x208
device_release_driver+0x14/0x20
bus_remove_device+0xcc/0x150
device_del+0x10c/0x310
device_unregister+0x1c/0x70
apr_remove_device+0xc/0x18
device_for_each_child+0x50/0x80
apr_remove+0x18/0x20
rpmsg_dev_remove+0x38/0x68
device_release_driver_internal+0x170/0x208
device_release_driver+0x14/0x20
bus_remove_device+0xcc/0x150
device_del+0x10c/0x310
device_unregister+0x1c/0x70
qcom_smd_remove_device+0xc/0x18
device_for_each_child+0x50/0x80
qcom_smd_unregister_edge+0x3c/0x70
smd_subdev_remove+0x18/0x28
rproc_stop+0x48/0xd8
rproc_shutdown+0x60/0xe8
state_store+0xbc/0xf8
dev_attr_store+0x18/0x28
sysfs_kf_write+0x3c/0x50
kernfs_fop_write+0x118/0x1e0
__vfs_write+0x18/0x110
vfs_write+0xa4/0x1a8
ksys_write+0x48/0xb0
sys_write+0xc/0x18
el0_svc_naked+0x30/0x34
Code: d2800022 8b400c21 f9800031 9ac32043 (c85f7c22)
---[ end trace 32020935775616a2 ]---
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
drivers/of/platform.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index c00d81dfac0b..78c32b93c0e7 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -544,8 +544,6 @@ int of_platform_device_destroy(struct device *dev, void *data)
amba_device_unregister(to_amba_device(dev));
#endif
- of_node_clear_flag(dev->of_node, OF_POPULATED);
- of_node_clear_flag(dev->of_node, OF_POPULATED_BUS);
return 0;
}
EXPORT_SYMBOL_GPL(of_platform_device_destroy);
--
2.16.2
^ permalink raw reply related
* Re: [PATCH v3 2/8] dt-bindings: media: Document data-enable-active property
From: Sakari Ailus @ 2018-06-01 22:35 UTC (permalink / raw)
To: jacopo mondi
Cc: devicetree, robh+dt, linux-renesas-soc, horms, geert,
laurent.pinchart, niklas.soderlund, Jacopo Mondi, mchehab,
hans.verkuil, linux-arm-kernel, linux-media
In-Reply-To: <20180601145827.GG10472@w540>
On Fri, Jun 01, 2018 at 04:58:27PM +0200, jacopo mondi wrote:
> Hi Sakari,
>
> On Fri, Jun 01, 2018 at 01:29:10PM +0300, Sakari Ailus wrote:
> > Hi Jacopo,
> >
> > Thanks for the patch.
> >
> > On Tue, May 29, 2018 at 05:05:53PM +0200, Jacopo Mondi wrote:
> > > Add 'data-enable-active' property to endpoint node properties list.
> > >
> > > The property allows to specify the polarity of the data-enable signal, which
> > > when in active state determinates when data lanes have to sampled for valid
> > > pixel data.
> >
> > Lanes or lines? I understand this is forthe parallel interface.
> >
>
> Now I'm confused. Are the parallel data 'lines' and the CSI-2 one
> 'lanes'? I thought 'lanes' applies to both.. am I wrong?
"Lane" is conventionally refer to a differential pair of wires on a serial
bus such as CSI-2. "Line" is used of a wire on a parallel bus.
--
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply
* Re: [PATCH v2 1/5] media: venus: add a routine to reset ARM9
From: Stanimir Varbanov @ 2018-06-01 22:15 UTC (permalink / raw)
To: Vikash Garodia, hverkuil, mchehab, robh, mark.rutland, andy.gross,
bjorn.andersson, stanimir.varbanov
Cc: linux-media, linux-kernel, linux-arm-msm, linux-soc, devicetree,
acourbot
In-Reply-To: <1527884768-22392-2-git-send-email-vgarodia@codeaurora.org>
Hi Vikash,
On 1.06.2018 23:26, Vikash Garodia wrote:
> Add a new routine to reset the ARM9 and brings it
> out of reset. This is in preparation to add PIL
> functionality in venus driver.
please squash this patch with 4/5. I don't see a reason to add a
function which is not used. Shouldn't this produce gcc warnings?
>
> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> ---
> drivers/media/platform/qcom/venus/firmware.c | 26 ++++++++++++++++++++++++
> drivers/media/platform/qcom/venus/hfi_venus_io.h | 5 +++++
> 2 files changed, 31 insertions(+)
>
> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
> index 521d4b3..7d89b5a 100644
> --- a/drivers/media/platform/qcom/venus/firmware.c
> +++ b/drivers/media/platform/qcom/venus/firmware.c
> @@ -14,6 +14,7 @@
>
> #include <linux/device.h>
> #include <linux/firmware.h>
> +#include <linux/delay.h>
> #include <linux/kernel.h>
> #include <linux/io.h>
> #include <linux/of.h>
> @@ -22,11 +23,36 @@
> #include <linux/sizes.h>
> #include <linux/soc/qcom/mdt_loader.h>
>
> +#include "core.h"
> #include "firmware.h"
> +#include "hfi_venus_io.h"
>
> #define VENUS_PAS_ID 9
> #define VENUS_FW_MEM_SIZE (6 * SZ_1M)
>
> +static void venus_reset_hw(struct venus_core *core)
can we rename this to venus_reset_cpu? reset_hw sounds like we reset
vcodec IPs, so I think we can be more exact here.
> +{
> + void __iomem *reg_base = core->base;
just 'base', please.
> +
> + writel(0, reg_base + WRAPPER_FW_START_ADDR);
> + writel(VENUS_FW_MEM_SIZE, reg_base + WRAPPER_FW_END_ADDR);
> + writel(0, reg_base + WRAPPER_CPA_START_ADDR);
> + writel(VENUS_FW_MEM_SIZE, reg_base + WRAPPER_CPA_END_ADDR);
> + writel(0x0, reg_base + WRAPPER_CPU_CGC_DIS);
> + writel(0x0, reg_base + WRAPPER_CPU_CLOCK_CONFIG);
> +
> + /* Make sure all register writes are committed. */
> + mb();
the comment says "register writes" hence shouldn't this be wmb? Also if
we are going to have explicit memory barrier why not use writel_relaxed?
> +
> + /*
> + * Need to wait 10 cycles of internal clocks before bringing ARM9
Do we know what is the minimum frequency of the internal ARM9 clocks?
I.e does 1us is enough for all venus versions.
> + * out of reset.
> + */
> + udelay(1);
> +
> + /* Bring Arm9 out of reset */
ARM9
> + writel_relaxed(0, reg_base + WRAPPER_A9SS_SW_RESET);
in my opinion we should have a wmb here too
regards,
Stan
^ permalink raw reply
* Re: [PATCH v2 1/2] power: supply: sbs-battery: don't assume MANUFACTURER_DATA formats
From: Rhyland Klein @ 2018-06-01 22:08 UTC (permalink / raw)
To: Brian Norris, Guenter Roeck
Cc: Sebastian Reichel, linux-kernel, Rob Herring, linux-pm,
devicetree, Alexandru Stan, Doug Anderson
In-Reply-To: <20180601183126.GA72050@rodete-desktop-imager.corp.google.com>
On 6/1/2018 2:31 PM, Brian Norris wrote:
> Hi,
>
> On Fri, Jun 01, 2018 at 10:34:34AM -0700, Guenter Roeck wrote:
>> On Fri, Jun 01, 2018 at 10:23:59AM -0700, Brian Norris wrote:
>>> drivers/power/supply/sbs-battery.c | 54 +++++++++++++++++++++++++-----
>>> 1 file changed, 46 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
>>> index 83d7b4115857..8dea63464a3f 100644
>>> --- a/drivers/power/supply/sbs-battery.c
>>> +++ b/drivers/power/supply/sbs-battery.c
> ...
>
>>> @@ -315,6 +320,27 @@ static int sbs_status_correct(struct i2c_client *client, int *intval)
>>> static int sbs_get_battery_presence_and_health(
>>> struct i2c_client *client, enum power_supply_property psp,
>>> union power_supply_propval *val)
>>> +{
>>> + int ret;
>>> +
>>> + if (psp == POWER_SUPPLY_PROP_PRESENT) {
>>> + /* Dummy command; if it succeeds, battery is present. */
>>> + ret = sbs_read_word_data(client, sbs_data[REG_STATUS].addr);
>>> + if (ret < 0)
>>> + val->intval = 0; /* battery disconnected */
>>> + else
>>> + val->intval = 1; /* battery present */
>>> + return 0;
>>> + } else { /* POWER_SUPPLY_PROP_HEALTH */
>>
>> Static analyzers may complain that else after return is unnecessary.
>
> I noticed (checkpatch complains) but decided I didn't care. It would be
> worse to promote the 'else' to top-level (things would look asymmetric).
> I suppose I could pull the 'return 0' out, but I'm not sure that would
> make the code any better.
I generally prefer to make checkpatch happy, so I would vote to move the
return outside of the if blocks as a minimal way of making it happy. In
general though,
Acked-by: Rhyland Klein <rklein@nvidia.com>
>
>> Other than that
>>
>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>>
>>> + /* SBS spec doesn't have a general health command. */
>>> + val->intval = POWER_SUPPLY_HEALTH_UNKNOWN;
>>> + return 0;
>>> + }
>>> +}
>>> +
>
> Brian
>
--
nvpublic
^ permalink raw reply
* Re: [PATCH v3 0/8] gnss: add new GNSS subsystem
From: Pavel Machek @ 2018-06-01 21:46 UTC (permalink / raw)
To: H. Nikolaus Schaller
Cc: Johan Hovold, Greg Kroah-Hartman, Rob Herring, Mark Rutland,
Andreas Kemnade, Arnd Bergmann, Marcel Holtmann,
Sebastian Reichel, Tony Lindgren, linux-kernel, devicetree
In-Reply-To: <3A356534-FEB0-4E0D-A8C8-922EEF1E0926@computer.org>
[-- Attachment #1: Type: text/plain, Size: 1163 bytes --]
On Fri 2018-06-01 18:37:36, H. Nikolaus Schaller wrote:
> Hi Pavel,
>
> > Am 01.06.2018 um 18:26 schrieb Pavel Machek <pavel@ucw.cz>:
> >
> > NMEA would not be my first choice, really. I'd propose something very
> > similar to existing /dev/input/eventX, but with wider data types.
>
> Since even Rome wasn't built in a day, my first choice is NMEA, but I am
> open to anything on higher level to come second.
>
> What about iio?
>
> It is quite flexible and extensible and GNSS coordinates are a not very
> different from accelerometer, gyroscope or compass data as all of them
> describe the position and orientation, maybe speed of the device these
> things are built into (at least for mobile devices).
As I said, I do not want to have that discussion at the
moment. Transfering position and orientation is easy, transfering some
other data (such as sattelites used for fix) might be
trickier. Anyway, I'm pretty sure reasonable solution exists; and yes
we could use NMEA.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v4 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings
From: David Collins @ 2018-06-01 21:41 UTC (permalink / raw)
To: Mark Brown
Cc: Doug Anderson, Liam Girdwood, Rob Herring, Mark Rutland,
linux-arm-msm, Linux ARM, devicetree, LKML, Rajendra Nayak,
Stephen Boyd
In-Reply-To: <20180531114816.GC13319@sirena.org.uk>
Hello Mark,
On 05/31/2018 04:48 AM, Mark Brown wrote:
> On Wed, May 30, 2018 at 04:39:10PM -0700, David Collins wrote:
>> The DRMS modes to use and max allowed current per mode need to be
>> specified at the board level in device tree instead of hard-coded per
>> regulator type in the driver. There are at least two use cases driving
>> this need: LDOs shared between RPMh client processors and SMPSes requiring
>> PWM mode in certain circumstances.
>
> Is there really no way to improve the RPM firmware?
This aggregation takes place in a discrete hardware block, not in a
general purpose processor. Theoretically, future chips could have
redesigned VRM hardware; however, there is no plan to make such a change.
>> Consider the case of a regulator with physical 10 mA LPM max current. Say
>> that modem and application processors each have a load on the regulator
>> that draws 9 mA. If they each respect the 10 mA limit, then they'd each
>> vote for LPM. The VRM block in RPMh hardware will aggregate these requests
>
> This is, of course, why the regulator API aggregates this stuff based on
> the current not based on having every individual user select a mode.
>
>> together using a max function which will result in the regulator being set
>> to LPM, even though the total load is 18 mA (which would require high
>> power mode (HPM)). To get around this corner case, a LPM max current of 1
>> uA can be used for all LDO supplies that have non-application processor
>> consumers. Thus, any non-zero regulator_set_load() current request will
>> result in setting the regulator to HPM (which is always safe).
>
> That's obviously just never going to work well, the best you can do here
> is just pretend that the other components are always operating at full
> power (which I assume all the other components are doing too...) or not
> try to use load based mode switching in the first place.
I will remove the DT-based mode and max load current mapping support. In
its place, I'll implement hard coded LPM current limits for LDOs of 10 mA
or 30 mA (depending upon subtype) like is done in other regulator drivers.
If we actually encounter an issue caused by the APPS processor and another
RPMh client both voting for LPM when HPM is needed for the combination,
then we can disable load-based mode control for the affected regulator in
DT and configure its initial mode as HPM.
>> The second situation that needs board-level DRMS mode and current limit
>> specification is SMPS regulator AUTO mode to PWM (HPM) mode switching.
>> SMPS regulators should theoretically always be able to operate in AUTO
>> mode as it switches automatically between PWM mode (which can provide the
>> maximum current) and PFM mode (which supports lower current but has higher
>> efficiency). However, there may be board/system issues that require
>> switching to PWM mode for certain use cases as it has better load
>> regulation (i.e. no PFM ripple for lower loads) and supports more
>> aggressive load current steps (i.e. greater A/ns).
>
>> If a Linux consumer requires the ability to force a given SMPS regulator
>> from AUTO mode into PWM mode and that SMPS is shared by other Linux
>> consumers (which may be the case, but at least must be guaranteed to work
>> architecturally), then regulator_set_load() is the only option since it
>> provides aggregation, where as regulator_set_mode() does not.
>
> That's obviously broken though, what you're describing is just clearly
> nothing to do with load so trying to configure it using load is just
> going to lead to problems later on. Honestly it sounds like you just
> want to force the regulator into forced PWM mode all the time, otherwise
> you need to look into implementing support for describing the thing
> you're actually trying to do and add a mechanism for per consumer mode
> configuration.
>
> This has been a frequent pattern with these RPM drivers, trying to find
> some way to shoehorn something that happens to work right now into the
> code. That's going to make things fragile and hard to maintain, we need
> code that does the thing it's saying it does so that it's easier to
> understand and work with - getting things running isn't enough, they
> need to be clear.
I agree that using regulator_set_load() to handle SMPS AUTO mode to PWM
mode switching is hacky. Since there is no natural way to pick SMPS modes
based on load current, I will remove the functionality completely. In its
place, we can configure the SMPSes to have an initial mode of AUTO. If a
use case requiring PWM mode arises, then the consumer driver responsible
for it can call regulator_set_mode() to switch between AUTO and PWM modes
explicitly.
Since regulator_set_mode() does not support aggregation currently, this
technique would work only if there is exactly one consumer per regulator
that needs explicit mode control. Should we hit a situation that needs
multiple consumers to have such mode control, then we could simply default
the SMPS to PWM mode.
I'll also start looking into per-consumer mode configuration and
regulator_set_mode() aggregation within the regulator framework. I think
that we should be able to function without it for now; however, it would
be good to have going forward.
Take care,
David
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v2 5/5] venus: register separate driver for firmware device
From: Jordan Crouse @ 2018-06-01 21:32 UTC (permalink / raw)
To: Vikash Garodia
Cc: hverkuil, mchehab, robh, mark.rutland, andy.gross,
bjorn.andersson, stanimir.varbanov, linux-media, linux-kernel,
linux-arm-msm, linux-soc, devicetree, acourbot
In-Reply-To: <1527884768-22392-6-git-send-email-vgarodia@codeaurora.org>
On Sat, Jun 02, 2018 at 01:56:08AM +0530, Vikash Garodia wrote:
> A separate child device is added for video firmware.
> This is needed to
> [1] configure the firmware context bank with the desired SID.
> [2] ensure that the iova for firmware region is from 0x0.
>
> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> ---
> .../devicetree/bindings/media/qcom,venus.txt | 8 +++-
> drivers/media/platform/qcom/venus/core.c | 48 +++++++++++++++++++---
> drivers/media/platform/qcom/venus/firmware.c | 20 ++++++++-
> drivers/media/platform/qcom/venus/firmware.h | 2 +
> 4 files changed, 71 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt b/Documentation/devicetree/bindings/media/qcom,venus.txt
> index 00d0d1b..701cbe8 100644
> --- a/Documentation/devicetree/bindings/media/qcom,venus.txt
> +++ b/Documentation/devicetree/bindings/media/qcom,venus.txt
> @@ -53,7 +53,7 @@
>
> * Subnodes
> The Venus video-codec node must contain two subnodes representing
> -video-decoder and video-encoder.
> +video-decoder and video-encoder, one optional firmware subnode.
>
> Every of video-encoder or video-decoder subnode should have:
>
> @@ -79,6 +79,8 @@ Every of video-encoder or video-decoder subnode should have:
> power domain which is responsible for collapsing
> and restoring power to the subcore.
>
> +The firmware sub node must contain the iommus specifiers for ARM9.
> +
> * An Example
> video-codec@1d00000 {
> compatible = "qcom,msm8916-venus";
> @@ -105,4 +107,8 @@ Every of video-encoder or video-decoder subnode should have:
> clock-names = "core";
> power-domains = <&mmcc VENUS_CORE1_GDSC>;
> };
> + venus-firmware {
> + compatible = "qcom,venus-firmware-no-tz";
> + iommus = <&apps_smmu 0x10b2 0x0>;
> + }
> };
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 101612b..5cfb3c2 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -179,6 +179,19 @@ static u32 to_v4l2_codec_type(u32 codec)
> }
> }
>
> +static int store_firmware_dev(struct device *dev, void *data)
> +{
> + struct venus_core *core = data;
> +
> + if (!core)
> + return -EINVAL;
Core is not going to be null here - you don't need to check it.
<snip>
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v2 4/5] media: venus: add no TZ boot and shutdown routine
From: Jordan Crouse @ 2018-06-01 21:30 UTC (permalink / raw)
To: Vikash Garodia
Cc: hverkuil, mchehab, robh, mark.rutland, andy.gross,
bjorn.andersson, stanimir.varbanov, linux-media, linux-kernel,
linux-arm-msm, linux-soc, devicetree, acourbot
In-Reply-To: <1527884768-22392-5-git-send-email-vgarodia@codeaurora.org>
On Sat, Jun 02, 2018 at 01:56:07AM +0530, Vikash Garodia wrote:
> Video hardware is mainly comprised of vcodec subsystem
> and video control subsystem. Video control has ARM9 which
> executes the video firmware instructions whereas vcodec
> does the video frame processing.
> This change adds support to load the video firmware and
> bring ARM9 out of reset for platforms which does not
> have trustzone.
>
> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> ---
> drivers/media/platform/qcom/venus/core.c | 6 +-
> drivers/media/platform/qcom/venus/core.h | 5 ++
> drivers/media/platform/qcom/venus/firmware.c | 86 ++++++++++++++++++++++++++--
> drivers/media/platform/qcom/venus/firmware.h | 7 ++-
> 4 files changed, 94 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 9a95f9a..101612b 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -76,7 +76,7 @@ static void venus_sys_error_handler(struct work_struct *work)
> hfi_core_deinit(core, true);
> hfi_destroy(core);
> mutex_lock(&core->lock);
> - venus_shutdown(core->dev);
> + venus_shutdown(core);
>
> pm_runtime_put_sync(core->dev);
>
> @@ -318,7 +318,7 @@ static int venus_probe(struct platform_device *pdev)
> err_core_deinit:
> hfi_core_deinit(core, false);
> err_venus_shutdown:
> - venus_shutdown(dev);
> + venus_shutdown(core);
> err_runtime_disable:
> pm_runtime_set_suspended(dev);
> pm_runtime_disable(dev);
> @@ -339,7 +339,7 @@ static int venus_remove(struct platform_device *pdev)
> WARN_ON(ret);
>
> hfi_destroy(core);
> - venus_shutdown(dev);
> + venus_shutdown(core);
> of_platform_depopulate(dev);
>
> pm_runtime_put_sync(dev);
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index e7bfb63..f04e25e 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -85,6 +85,10 @@ struct venus_caps {
> bool valid;
> };
>
> +struct video_firmware {
> + struct device *dev;
> + struct iommu_domain *iommu_domain;
> +};
> /**
> * struct venus_core - holds core parameters valid for all instances
> *
> @@ -129,6 +133,7 @@ struct venus_core {
> struct device *dev;
> struct device *dev_dec;
> struct device *dev_enc;
> + struct video_firmware fw;
> struct mutex lock;
> struct list_head instances;
> atomic_t insts_count;
> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
> index cb7f48ef..058d544 100644
> --- a/drivers/media/platform/qcom/venus/firmware.c
> +++ b/drivers/media/platform/qcom/venus/firmware.c
> @@ -12,8 +12,10 @@
> *
> */
>
> +#include <linux/platform_device.h>
> #include <linux/device.h>
> #include <linux/firmware.h>
> +#include <linux/iommu.h>
> #include <linux/delay.h>
> #include <linux/kernel.h>
> #include <linux/io.h>
> @@ -27,9 +29,6 @@
> #include "firmware.h"
> #include "hfi_venus_io.h"
>
> -#define VENUS_PAS_ID 9
> -#define VENUS_FW_MEM_SIZE (6 * SZ_1M)
> -
> static void venus_reset_hw(struct venus_core *core)
> {
> void __iomem *reg_base = core->base;
> @@ -125,7 +124,7 @@ static int venus_load_fw(struct device *dev, const char *fwname,
> }
> if (qcom_scm_is_available())
> ret = qcom_mdt_load(dev, mdt, fwname, VENUS_PAS_ID, mem_va,
> - *mem_phys, *mem_size, NULL);
> + *mem_phys, *mem_size);
> else
> ret = qcom_mdt_load_no_init(dev, mdt, fwname, VENUS_PAS_ID,
> mem_va, *mem_phys, *mem_size, NULL);
> @@ -136,6 +135,77 @@ static int venus_load_fw(struct device *dev, const char *fwname,
> memunmap(mem_va);
> return ret;
> }
> +
> +int venus_boot_noTZ(struct venus_core *core, phys_addr_t mem_phys,
I'm not super enthusiastic about the capital letters, but I know what you're
going for so if everybody else is cool with it, I can be too.
> + size_t mem_size)
> +{
> + struct iommu_domain *iommu;
> + struct device *dev;
> + int ret;
> +
> + if (!core->fw.dev)
> + return -EPROBE_DEFER;
> +
> + dev = core->fw.dev;
> +
> + iommu = iommu_domain_alloc(&platform_bus_type);
> + if (!iommu) {
> + dev_err(dev, "Failed to allocate iommu domain\n");
> + return -ENOMEM;
> + }
> +
> + ret = iommu_attach_device(iommu, dev);
> + if (ret) {
> + dev_err(dev, "could not attach device\n");
Should be "could not attach device to iommu", so you know what part of the code you
are in.
> + goto err_attach;
> + }
> +
> + ret = iommu_map(iommu, VENUS_FW_START_ADDR, mem_phys, mem_size,
> + IOMMU_READ|IOMMU_WRITE|IOMMU_PRIV);
Do you really want to be mapping your firmware memory as IOMMU_WRITE?
> + if (ret) {
> + dev_err(dev, "could not map video firmware region\n");
> + goto err_map;
> + }
> + core->fw.iommu_domain = iommu;
> + venus_reset_hw(core);
> +
> + return 0;
> +
> +err_map:
> + iommu_detach_device(iommu, dev);
> +err_attach:
> + iommu_domain_free(iommu);
> + return ret;
> +}
> +
> +int venus_shutdown_noTZ(struct venus_core *core)
> +{
> + struct iommu_domain *iommu;
> + size_t unmapped = 0;
> + u32 reg;
> + struct device *dev = core->fw.dev;
> + void __iomem *reg_base = core->base;
> +
> + /* Assert the reset to ARM9 */
> + reg = readl_relaxed(reg_base + WRAPPER_A9SS_SW_RESET);
> + reg |= BIT(4);
> + writel_relaxed(reg, reg_base + WRAPPER_A9SS_SW_RESET);
You could just use core->base directly in both these cases and not bother with
the local variable.
> +
> + /* Make sure reset is asserted before the mapping is removed */
> + mb();
> +
> + iommu = core->fw.iommu_domain;
> +
> + unmapped = iommu_unmap(iommu, VENUS_FW_START_ADDR, VENUS_FW_MEM_SIZE);
> + if (unmapped != VENUS_FW_MEM_SIZE)
> + dev_err(dev, "failed to unmap firmware\n");
> +
> + iommu_detach_device(iommu, dev);
> + iommu_domain_free(iommu);
> +
> + return 0;
> +}
> +
> int venus_boot(struct venus_core *core)
> {
> phys_addr_t mem_phys;
> @@ -156,16 +226,20 @@ int venus_boot(struct venus_core *core)
>
> if (qcom_scm_is_available())
> ret = qcom_scm_pas_auth_and_reset(VENUS_PAS_ID);
> + else
> + ret = venus_boot_noTZ(core, mem_phys, mem_size);
>
> return ret;
> }
> -EXPORT_SYMBOL_GPL(venus_boot);
>
> -int venus_shutdown(struct device *dev)
> +int venus_shutdown(struct venus_core *core)
> {
> int ret = 0;
>
> if (qcom_scm_is_available())
> ret = qcom_scm_pas_shutdown(VENUS_PAS_ID);
> + else
> + ret = venus_shutdown_noTZ(core);
> +
> return ret;
> }
> diff --git a/drivers/media/platform/qcom/venus/firmware.h b/drivers/media/platform/qcom/venus/firmware.h
> index 0916826..67fdd89 100644
> --- a/drivers/media/platform/qcom/venus/firmware.h
> +++ b/drivers/media/platform/qcom/venus/firmware.h
> @@ -14,10 +14,15 @@
> #ifndef __VENUS_FIRMWARE_H__
> #define __VENUS_FIRMWARE_H__
>
> +#define VENUS_PAS_ID 9
> +#define VENUS_FW_START_ADDR 0x0
> +#define VENUS_FW_MEM_SIZE (6 * SZ_1M)
> +#define VENUS_MAX_MEM_REGION 0xE0000000
> +
> struct device;
>
> int venus_boot(struct venus_core *core);
> -int venus_shutdown(struct device *dev);
> +int venus_shutdown(struct venus_core *core);
> int venus_set_hw_state(enum tzbsp_video_state, struct venus_core *core);
>
> #endif
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v2 3/5] venus: add check to make scm calls
From: Jordan Crouse @ 2018-06-01 21:22 UTC (permalink / raw)
To: Vikash Garodia
Cc: hverkuil, mchehab, robh, mark.rutland, andy.gross,
bjorn.andersson, stanimir.varbanov, linux-media, linux-kernel,
linux-arm-msm, linux-soc, devicetree, acourbot
In-Reply-To: <1527884768-22392-4-git-send-email-vgarodia@codeaurora.org>
On Sat, Jun 02, 2018 at 01:56:06AM +0530, Vikash Garodia wrote:
> Split the boot api into firmware load and hardware
> boot. Also add the checks to invoke scm calls only
> if the platform has the required support.
>
> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> ---
> drivers/media/platform/qcom/venus/core.c | 4 +-
> drivers/media/platform/qcom/venus/firmware.c | 65 ++++++++++++++++++----------
> drivers/media/platform/qcom/venus/firmware.h | 2 +-
> 3 files changed, 45 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 1308488..9a95f9a 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -84,7 +84,7 @@ static void venus_sys_error_handler(struct work_struct *work)
>
> pm_runtime_get_sync(core->dev);
>
> - ret |= venus_boot(core->dev, core->res->fwname);
> + ret |= venus_boot(core);
>
> ret |= hfi_core_resume(core, true);
>
> @@ -279,7 +279,7 @@ static int venus_probe(struct platform_device *pdev)
> if (ret < 0)
> goto err_runtime_disable;
>
> - ret = venus_boot(dev, core->res->fwname);
> + ret = venus_boot(core);
> if (ret)
> goto err_runtime_disable;
>
> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
> index b4664ed..cb7f48ef 100644
> --- a/drivers/media/platform/qcom/venus/firmware.c
> +++ b/drivers/media/platform/qcom/venus/firmware.c
> @@ -81,40 +81,35 @@ int venus_set_hw_state(enum tzbsp_video_state state, struct venus_core *core)
> }
> EXPORT_SYMBOL_GPL(venus_set_hw_state);
>
> -int venus_boot(struct device *dev, const char *fwname)
> +static int venus_load_fw(struct device *dev, const char *fwname,
> + phys_addr_t *mem_phys, size_t *mem_size)
> {
> const struct firmware *mdt;
> struct device_node *node;
> - phys_addr_t mem_phys;
> struct resource r;
> ssize_t fw_size;
> - size_t mem_size;
> void *mem_va;
> int ret;
>
> - if (!IS_ENABLED(CONFIG_QCOM_MDT_LOADER) || !qcom_scm_is_available())
> - return -EPROBE_DEFER;
> -
> node = of_parse_phandle(dev->of_node, "memory-region", 0);
> if (!node) {
> dev_err(dev, "no memory-region specified\n");
> return -EINVAL;
> }
> -
Unrelated whitespace change. Not needed.
> ret = of_address_to_resource(node, 0, &r);
> if (ret)
> return ret;
>
> - mem_phys = r.start;
> - mem_size = resource_size(&r);
> + *mem_phys = r.start;
> + *mem_size = resource_size(&r);
>
> - if (mem_size < VENUS_FW_MEM_SIZE)
> + if (*mem_size < VENUS_FW_MEM_SIZE)
> return -EINVAL;
>
> - mem_va = memremap(r.start, mem_size, MEMREMAP_WC);
> + mem_va = memremap(r.start, *mem_size, MEMREMAP_WC);
> if (!mem_va) {
> dev_err(dev, "unable to map memory region: %pa+%zx\n",
> - &r.start, mem_size);
> + &r.start, *mem_size);
> return -ENOMEM;
> }
>
> @@ -128,25 +123,49 @@ int venus_boot(struct device *dev, const char *fwname)
> release_firmware(mdt);
> goto err_unmap;
> }
> -
> - ret = qcom_mdt_load(dev, mdt, fwname, VENUS_PAS_ID, mem_va, mem_phys,
> - mem_size);
> + if (qcom_scm_is_available())
> + ret = qcom_mdt_load(dev, mdt, fwname, VENUS_PAS_ID, mem_va,
> + *mem_phys, *mem_size, NULL);
> + else
> + ret = qcom_mdt_load_no_init(dev, mdt, fwname, VENUS_PAS_ID,
> + mem_va, *mem_phys, *mem_size, NULL);
>
> release_firmware(mdt);
>
> - if (ret)
> - goto err_unmap;
> -
> - ret = qcom_scm_pas_auth_and_reset(VENUS_PAS_ID);
> - if (ret)
> - goto err_unmap;
> -
> err_unmap:
> memunmap(mem_va);
> return ret;
> }
> +int venus_boot(struct venus_core *core)
> +{
> + phys_addr_t mem_phys;
> + size_t mem_size;
> + int ret;
> + struct device *dev;
> +
> + if (!IS_ENABLED(CONFIG_QCOM_MDT_LOADER))
> + return -EPROBE_DEFER;
> +
> + dev = core->dev;
> +
> + ret = venus_load_fw(dev, core->res->fwname, &mem_phys, &mem_size);
> + if (ret) {
> + dev_err(dev, "fail to load video firmware\n");
> + return -EINVAL;
> + }
> +
> + if (qcom_scm_is_available())
> + ret = qcom_scm_pas_auth_and_reset(VENUS_PAS_ID);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(venus_boot);
>
> int venus_shutdown(struct device *dev)
> {
> - return qcom_scm_pas_shutdown(VENUS_PAS_ID);
> + int ret = 0;
> +
> + if (qcom_scm_is_available())
> + ret = qcom_scm_pas_shutdown(VENUS_PAS_ID);
> + return ret;
> }
> diff --git a/drivers/media/platform/qcom/venus/firmware.h b/drivers/media/platform/qcom/venus/firmware.h
> index 1336729..0916826 100644
> --- a/drivers/media/platform/qcom/venus/firmware.h
> +++ b/drivers/media/platform/qcom/venus/firmware.h
> @@ -16,7 +16,7 @@
>
> struct device;
>
> -int venus_boot(struct device *dev, const char *fwname);
> +int venus_boot(struct venus_core *core);
> int venus_shutdown(struct device *dev);
> int venus_set_hw_state(enum tzbsp_video_state, struct venus_core *core);
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v2 2/5] media: venus: add a routine to set venus state
From: Jordan Crouse @ 2018-06-01 21:21 UTC (permalink / raw)
To: Vikash Garodia
Cc: hverkuil, mchehab, robh, mark.rutland, andy.gross,
bjorn.andersson, stanimir.varbanov, linux-media, linux-kernel,
linux-arm-msm, linux-soc, devicetree, acourbot
In-Reply-To: <1527884768-22392-3-git-send-email-vgarodia@codeaurora.org>
On Sat, Jun 02, 2018 at 01:56:05AM +0530, Vikash Garodia wrote:
> Add a new routine which abstracts the TZ call to
> set the video hardware state.
>
> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> ---
> drivers/media/platform/qcom/venus/core.h | 5 +++++
> drivers/media/platform/qcom/venus/firmware.c | 28 +++++++++++++++++++++++++++
> drivers/media/platform/qcom/venus/firmware.h | 1 +
> drivers/media/platform/qcom/venus/hfi_venus.c | 13 ++++---------
> 4 files changed, 38 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index 85e66e2..e7bfb63 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -35,6 +35,11 @@ struct reg_val {
> u32 value;
> };
>
> +enum tzbsp_video_state {
> + TZBSP_VIDEO_SUSPEND = 0,
> + TZBSP_VIDEO_RESUME
You should put a comma after the last item to reduce future patch sizes.
> +};
> +
> struct venus_resources {
> u64 dma_mask;
> const struct freq_tbl *freq_tbl;
> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
> index 7d89b5a..b4664ed 100644
> --- a/drivers/media/platform/qcom/venus/firmware.c
> +++ b/drivers/media/platform/qcom/venus/firmware.c
> @@ -53,6 +53,34 @@ static void venus_reset_hw(struct venus_core *core)
> /* Bring Arm9 out of reset */
> writel_relaxed(0, reg_base + WRAPPER_A9SS_SW_RESET);
> }
> +
> +int venus_set_hw_state(enum tzbsp_video_state state, struct venus_core *core)
> +{
> + int ret;
> + struct device *dev = core->dev;
If you get rid of the log message as you should, you don't need this.
> + void __iomem *reg_base = core->base;
> +
> + switch (state) {
> + case TZBSP_VIDEO_SUSPEND:
> + if (qcom_scm_is_available())
> + ret = qcom_scm_set_remote_state(TZBSP_VIDEO_SUSPEND, 0);
> + else
> + writel_relaxed(1, reg_base + WRAPPER_A9SS_SW_RESET);
You can just use core->base here and not bother making a local variable for it.
> + break;
> + case TZBSP_VIDEO_RESUME:
> + if (qcom_scm_is_available())
> + ret = qcom_scm_set_remote_state(TZBSP_VIDEO_RESUME, 0);
> + else
> + venus_reset_hw(core);
> + break;
> + default:
> + dev_err(dev, "invalid state\n");
state is a enum - you are highly unlikely to be calling it in your own code with
a random value. It is smart to have the default, but you don't need the log
message - that is just wasted space in the binary.
> + break;
> + }
There are three paths in the switch statement that could end up with 'ret' being
uninitialized here. Set it to 0 when you declare it.
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(venus_set_hw_state);
> +
> int venus_boot(struct device *dev, const char *fwname)
> {
> const struct firmware *mdt;
> diff --git a/drivers/media/platform/qcom/venus/firmware.h b/drivers/media/platform/qcom/venus/firmware.h
> index 428efb5..1336729 100644
> --- a/drivers/media/platform/qcom/venus/firmware.h
> +++ b/drivers/media/platform/qcom/venus/firmware.h
> @@ -18,5 +18,6 @@
>
> int venus_boot(struct device *dev, const char *fwname);
> int venus_shutdown(struct device *dev);
> +int venus_set_hw_state(enum tzbsp_video_state, struct venus_core *core);
>
> #endif
> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> index f61d34b..797a9f5 100644
> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> @@ -19,7 +19,6 @@
> #include <linux/interrupt.h>
> #include <linux/iopoll.h>
> #include <linux/kernel.h>
> -#include <linux/qcom_scm.h>
> #include <linux/slab.h>
>
> #include "core.h"
> @@ -27,6 +26,7 @@
> #include "hfi_msgs.h"
> #include "hfi_venus.h"
> #include "hfi_venus_io.h"
> +#include "firmware.h"
>
> #define HFI_MASK_QHDR_TX_TYPE 0xff000000
> #define HFI_MASK_QHDR_RX_TYPE 0x00ff0000
> @@ -55,11 +55,6 @@
> #define IFACEQ_VAR_LARGE_PKT_SIZE 512
> #define IFACEQ_VAR_HUGE_PKT_SIZE (1024 * 12)
>
> -enum tzbsp_video_state {
> - TZBSP_VIDEO_STATE_SUSPEND = 0,
> - TZBSP_VIDEO_STATE_RESUME
> -};
> -
> struct hfi_queue_table_header {
> u32 version;
> u32 size;
> @@ -574,7 +569,7 @@ static int venus_power_off(struct venus_hfi_device *hdev)
> if (!hdev->power_enabled)
> return 0;
>
> - ret = qcom_scm_set_remote_state(TZBSP_VIDEO_STATE_SUSPEND, 0);
> + ret = venus_set_hw_state(TZBSP_VIDEO_SUSPEND, hdev->core);
> if (ret)
> return ret;
>
> @@ -594,7 +589,7 @@ static int venus_power_on(struct venus_hfi_device *hdev)
> if (hdev->power_enabled)
> return 0;
>
> - ret = qcom_scm_set_remote_state(TZBSP_VIDEO_STATE_RESUME, 0);
> + ret = venus_set_hw_state(TZBSP_VIDEO_RESUME, hdev->core);
> if (ret)
> goto err;
>
> @@ -607,7 +602,7 @@ static int venus_power_on(struct venus_hfi_device *hdev)
> return 0;
>
> err_suspend:
> - qcom_scm_set_remote_state(TZBSP_VIDEO_STATE_SUSPEND, 0);
> + venus_set_hw_state(TZBSP_VIDEO_SUSPEND, hdev->core);
> err:
> hdev->power_enabled = false;
> return ret;
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v8 2/4] dt-bindings: Add bindings for SPI NAND devices
From: Boris Brezillon @ 2018-06-01 21:07 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Pawel Moll, Ian Campbell, Richard Weinberger, Kumar Gala,
Rob Herring, linux-spi, Marek Vasut, Mark Brown, MTD Maling List,
Miquel Raynal, Brian Norris, David Woodhouse
In-Reply-To: <CAMuHMdWqa4++tdrpWCAsdqjqN26CHB1+eudnCtsP2vcrzmkA_w@mail.gmail.com>
On Fri, 1 Jun 2018 19:40:00 +0200
Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Boris,
>
> On Fri, Jun 1, 2018 at 7:09 PM, Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> > On Fri, 1 Jun 2018 16:42:26 +0200
> > Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >> On Fri, Jun 1, 2018 at 3:13 PM, Boris Brezillon
> >> <boris.brezillon@bootlin.com> wrote:
> >> > Add bindings for SPI NAND chips.
>
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/mtd/spi-nand.txt
> >> > @@ -0,0 +1,27 @@
> >> > +SPI NAND flash
> >> > +
> >> > +Required properties:
> >> > +- compatible: should be "spi-nand"
> >> > +- reg: should encode the chip-select line used to access the NAND chip
> >> > +
> >> > +Optional properties
> >> > +- spi-max-frequency: maximum frequency of the SPI bus the chip can operate at.
> >> > + This should encode board limitations (i.e. max freq can't
> >> > + be achieved due to crosstalk on IO lines).
> >> > + When unspecified, the driver assumes the chip can run at
> >> > + the max frequency defined in the spec (information
> >> > + extracted chip detection time).
> >>
> >> This is a standard property according to
> >> Documentation/devicetree/bindings/spi/spi-bus.txt. Can't you just refer
> >> to that file, or just omit it, as it applies to all SPI slaves anyway?
> >
> > The thing is, the maximum frequency supported by a SPI NAND is directly
> > encoded in the NAND device ID and can be auto-detected. Why should we
> > define spi-max-frequency in the DT when we can automatically detect
> > this information? The only reason one might want to override
>
> Because that's how we dealt with this traditionally. Or at least I thought so.
> But include/linux/spi/spi.h says:
>
> * @max_speed_hz: Maximum clock rate to be used with this chip
> * (on this board); may be changed by the device's driver.
> * The spi_transfer.speed_hz can override this for each transfer.
>
> and many drivers seem to do so.
And that's wrong, isn't it? I mean, if you have a constraint at the
board level, why should we allow the SPI device driver to override this
value? We should have:
max(controller_max, device_max, board_max)
and not (this is my understanding of how SPI max-freq selection works)
if (board_max)
max = board_max;
else
max = controller_max;
if (device_max)
max = device_max;
>
> > spi-max-frequency is when the board design impose such restrictions,
> > hence the precision I give here.
>
> Which is exactly the meaning of the standard property, isn't it?
Maybe it is how people expect the max-freq selection to work, but in
practice it doesn't seem to work like that (see spi_setup() and how the
value passed by the device driver overrides everything without even
checking if the controller and board constraints impose a lower freq).
>
> So I think it can just be omitted here anyway. If it's present, the SPI
> core will make sure it's taken into account.
Yes, I should probably just refer to spi-bus.txt here.
>
> >> > +- spi-tx-bus-width: The bus width (number of data wires) that is used for MOSI.
> >> > + Only encodes the board constraints (i.e. when not all IO
> >> > + signals are routed on the board). Device constraints are
> >> > + extracted when detecting the chip, and controller
> >> > + constraints are exposed by the SPI mem controller. If this
> >> > + property is missing that means no constraint at the board
> >> > + level.
> >> > +- spi-rx-bus-width: The bus width (number of data wires) that is used for MISO.
> >> > + Only encodes the board constraints (i.e. when not all IO
> >> > + signals are routed on the board). Device constraints are
> >> > + extracted when detecting the chip, and controller
> >> > + constraints are exposed by the SPI mem controller. If this
> >> > + property is missing that means no constraint at the board
> >> > + level.
> >>
> >> This does not match Documentation/devicetree/bindings/spi/spi-bus.txt,
> >> which says the default is 1.
> >
> > Yes, I know.
> >
> >> As these properties are handled by the SPI core in of_spi_parse_dt, why
> >> would you want to deviate?
> >
> > Because, again, this information can be extracted from the NAND ID, and
> > the only reason we might want to override the information extracted
> > from the NAND ID is when the board design adds extra restrictions
> > (like, only 2 SIO lines wired on the 4 available).
>
> In struct spi_device, this is specified using the SPI_[RT]_X_{DUAL,QUAD}
> bits in the mode field. Documentation says:
>
> * @mode: The spi mode defines how data is clocked out and in.
> * This may be changed by the device's driver.
> * The "active low" default for chipselect mode can be overridden
> * (by specifying SPI_CS_HIGH) as can the "MSB first" default for
> * each word in a transfer (by specifying SPI_LSB_FIRST).
>
> So this may also be specified by the device driver, but it seems no driver
> does so for the dual/quad bits, except for drivers/mtd/spi-nor/nxp-spifi.c
> (for rx only).
Same problem as for spi-max-frequency, the driver can override the value
that was specified in the DT, and the core does not seem to check
if the board or controller constraint were stronger.
>
> But here we do have the generic SPI DT bindings saying the default is 1.
> So personally, I wouldn't go for the option of the least surprise, and
> don't deviate from the generic bindings.
I do have a problem with this approach: we are in the process of
converting existing spi-nor controller drivers to the SPI mem model in
order to allow the same controller to indifferently control a SPI NAND
or a SPI NOR. Such drivers had their own bindings which most of the
time was close to the bindings described in spi-bus.txt with a few
exceptions. One of these exception is that drivers tend to not
explicitly describe the bus width but still use the max buswidth
supported by the controller (QuadSPI) to interface with NOR chips.
When we convert those drivers we have 2 choices:
1/ make existing setup work in a degraded mode (1-1-1) until they update
their DT to explicitly specify the spi-{rx,tx}-bus-width
2/ consider that when spi-{rx,tx}-bus-width is missing the device should
work in the fastest possible mode matching the device+controller
constraints
I went for option #2, but maybe #1 is fine. Note that spi-nand is not a
problem here because this is a new binding, but I thought it would be
better to clarify the meaning of these props before people start
(ab)using them.
>
> >> Commenting to the question in the cover letter: what would be the
> >> purpose of spi-max-bus-width?
> >
> > Defining how many IO lines are wired on the board design. If this
> > property is missing we would assume all IO lines are wired and the
> > restrictions would be negotiated between the controller and the device
> > without requiring explicit description in the DT.
>
> Which is exactly the meaning of the standard property, except for the
> default of all vs. 1.
And that's a huge difference. Say you have a board design and several
equivalence for the SPI NAND chip, one is supporting mode 1-1-4 (AKA
QuadSPI) and the other one is supporting only mode 1-1-1 (Single SPI).
With your solution we'd have to have 2 DTs or force all models to
operate in 1-1-1 mode.
>
> I'll have to defer to Mark (broonie) for his opinion about deviating from
> the way this is handled traditionally, and assuming different defaults...
I'm not saying that we should blindly change the meaning of those
existing props, I just wanted to start a discussion to see how the
problem I'm mentioning here should be handled (addition of new props is
something I'm perfectly fine with).
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply
* Re: [RFC PATCH 0/8] coresight: Update device tree bindings
From: Mathieu Poirier @ 2018-06-01 21:04 UTC (permalink / raw)
To: Suzuki K Poulose
Cc: linux-arm-kernel, sudeep.holla, robh, mark.rutland, frowand.list,
matt.sealey, charles.garcia-tobin, john.horley, mike.leach,
coresight, linux-kernel, devicetree
In-Reply-To: <1527858967-16047-1-git-send-email-suzuki.poulose@arm.com>
On Fri, Jun 01, 2018 at 02:15:59PM +0100, Suzuki K Poulose wrote:
> Coresight uses DT graph bindings to describe the connections of the
> components. However we have some undocumented usage of the bindings
> to describe some of the properties of the connections.
>
> The coresight driver needs to know the hardware ports invovled
> in the connection and the direction of data flow to effectively
> manage the trace sessions. So far we have relied on the "port"
> address (as described by the generic graph bindings) to represent
> the hardware port of the component for a connection.
>
> The hardware uses separate numbering scheme for input and output
> ports, which implies, we could have two different (input and output)
> ports with the same port number. This could create problems in the
> graph bindings where the label of the port wouldn't match the address.
>
> e.g, with the existing bindings we get :
>
> port@0{ // Output port 0
> reg = <0>;
> ...
> };
>
> port@1{
> reg = <0>; // Input port 0
> endpoint {
> slave-mode;
> ...
> };
> };
>
> With the new enforcement in the DT rules, mismatches in label and address
> are not allowed (as see in the case for port@1). So, we need a new mechanism
> to describe the hardware port number reliably.
>
> Also, we relied on an undocumented "slave-mode" property (see the above
> example) to indicate if the port is an input port. Let us formalise and
> switch to a new property to describe the direction of data flow.
>
> There were three options considered for the hardware port number scheme:
>
> 1) Use natural ordering in the DT to infer the hardware port number.
> i.e, Mandate that the all ports are listed in the DT and in the ascending
> order for each class (input and output respectively).
> Pros :
> - We don't need new properties and if the existing DTS list them in
> order (which most of them do), they work out of the box.
> Cons :
> - We must list all the ports even if the system cannot/shouldn't use
> it.
> - It is prone to human errors (if the order is not kept).
>
> 2) Use an explicit property to list both the direction and the hw port
> number and direction. Define "coresight,hwid" as 2 member array of u32,
> where the members are port number and the direction respectively.
> e.g
>
> port@0{
> reg = <0>;
> endpoint {
> coresight,hwid = <0 1>; // Port # 0, Output
> }
> };
>
> port@1{
> reg = <1>;
> endpoint {
> coresight,hwid = <0 0>; // Port # 0, Input
> };
> };
>
> Pros:
> - The bindings are formal but not so reader friendly and could potentially
> lead to human errors.
> Cons:
> - Backward compatiblity is lost.
> 3) Use explicit properties (implemented in the series) for the hardware
> port id and direction. We define a new property "coresight,hwid" for
> each endpoint in coresight devices to specify the hardware port number
> explicitly. Also use a separate property "direction" to specify the
> direction of the data flow.
>
> e.g,
>
> port@0{
> reg = <0>;
> endpoint {
> direction = <1>; // Output
> coresight,hwid = <0>; // Port # 0
> }
> };
>
> port@1{
> reg = <1>;
> endpoint {
> direction = <0>; // Input
> coresight,hwid = <0>; // Port # 0
> };
> };
>
> Pros:
> - The bindings are formal and reader friendly, and less prone to errors.
> Cons:
> - Backward compatibility is lost.
>
>
> This series achieves implements Option (3) listed above while still retaining
> the backward compatibility. The driver now issues a warning (once) when it
> encounters the old bindings.
> It also cleans up the platform parsing code to reduce the memory usage by
> reusing the platform description. The series also includes the
> changes for Juno platform as an example. If there are no objections
> to the approach, I could post the series, converting all the
> in-kernel DTS to the new binding.
>
> Suzuki K Poulose (8):
> dts: binding: coresight: Document graph bindings
> coresight: Fix remote endpoint parsing
> coresight: Cleanup platform description data
> coresight: platform: Cleanup coresight connection handling
> coresight: Handle errors in finding input/output ports
> dts: coresight: Clean up the device tree graph bindings
> dts: coresight: Define new bindings for direction of data flow
> dts: juno: Update coresight bindings for hw port
>
> .../devicetree/bindings/arm/coresight.txt | 52 ++++++++--
> arch/arm64/boot/dts/arm/juno-base.dtsi | 82 +++++++++++----
> arch/arm64/boot/dts/arm/juno.dts | 5 +-
> drivers/hwtracing/coresight/coresight.c | 28 ++----
> drivers/hwtracing/coresight/of_coresight.c | 111 ++++++++++++---------
> include/linux/coresight.h | 11 +-
> 6 files changed, 181 insertions(+), 108 deletions(-)
Aside from the comments I've already posted I'm pretty much good with this set.
Please rebase the next revision on my "next" branch and run checkpatch.pl on the
set. Patch 6/8 and 7/8 are generating warnings.
Thanks,
Mathieu
>
> --
> 2.7.4
>
^ permalink raw reply
* Re: [RFC PATCH 8/8] dts: juno: Update coresight bindings for hw port
From: Mathieu Poirier @ 2018-06-01 20:59 UTC (permalink / raw)
To: Suzuki K Poulose
Cc: linux-arm-kernel, sudeep.holla, robh, mark.rutland, frowand.list,
matt.sealey, charles.garcia-tobin, john.horley, mike.leach,
coresight, linux-kernel, devicetree, Liviu Dudau
In-Reply-To: <1527858967-16047-9-git-send-email-suzuki.poulose@arm.com>
On Fri, Jun 01, 2018 at 02:16:07PM +0100, Suzuki K Poulose wrote:
> Switch to updated coresight bindings for hw ports.
>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Reviewed-and-tested-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
> arch/arm64/boot/dts/arm/juno-base.dtsi | 82 +++++++++++++++++++++++++---------
> arch/arm64/boot/dts/arm/juno.dts | 5 ++-
> 2 files changed, 63 insertions(+), 24 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/arm/juno-base.dtsi b/arch/arm64/boot/dts/arm/juno-base.dtsi
> index eb749c5..33b41ba 100644
> --- a/arch/arm64/boot/dts/arm/juno-base.dtsi
> +++ b/arch/arm64/boot/dts/arm/juno-base.dtsi
> @@ -122,15 +122,18 @@
> port@0 {
> reg = <0>;
> etf0_in_port: endpoint {
> - slave-mode;
> + direction = <0>;
> remote-endpoint = <&main_funnel_out_port>;
> + coresight,hwid = <0>;
> };
> };
>
> /* output port */
> port@1 {
> - reg = <0>;
> + reg = <1>;
> etf0_out_port: endpoint {
> + coresight,hwid = <0>;
> + direction = <1>;
> };
> };
> };
> @@ -145,8 +148,9 @@
> power-domains = <&scpi_devpd 0>;
> port {
> tpiu_in_port: endpoint {
> - slave-mode;
> + direction = <0>;
> remote-endpoint = <&replicator_out_port0>;
> + coresight,hwid = <0>;
> };
> };
> };
> @@ -168,23 +172,27 @@
> reg = <0>;
> main_funnel_out_port: endpoint {
> remote-endpoint = <&etf0_in_port>;
> + coresight,hwid = <0>;
> + direction = <1>;
> };
> };
>
> /* input ports */
> port@1 {
> - reg = <0>;
> + reg = <1>;
> main_funnel_in_port0: endpoint {
> - slave-mode;
> + direction = <0>;
> remote-endpoint = <&cluster0_funnel_out_port>;
> + coresight,hwid = <0>;
> };
> };
>
> port@2 {
> - reg = <1>;
> + reg = <2>;
> main_funnel_in_port1: endpoint {
> - slave-mode;
> + direction = <0>;
> remote-endpoint = <&cluster1_funnel_out_port>;
> + coresight,hwid = <1>;
> };
> };
> };
> @@ -200,8 +208,9 @@
> power-domains = <&scpi_devpd 0>;
> port {
> etr_in_port: endpoint {
> - slave-mode;
> + direction = <0>;
> remote-endpoint = <&replicator_out_port1>;
> + coresight,hwid = <0>;
> };
> };
> };
> @@ -217,6 +226,8 @@
> power-domains = <&scpi_devpd 0>;
> port {
> stm_out_port: endpoint {
> + coresight,hwid = <0>;
> + direction = <1>;
> };
> };
> };
> @@ -240,6 +251,8 @@
> port {
> cluster0_etm0_out_port: endpoint {
> remote-endpoint = <&cluster0_funnel_in_port0>;
> + coresight,hwid = <0>;
> + direction = <1>;
> };
> };
> };
> @@ -259,22 +272,26 @@
> reg = <0>;
> cluster0_funnel_out_port: endpoint {
> remote-endpoint = <&main_funnel_in_port0>;
> + coresight,hwid = <0>;
> + direction = <1>;
> };
> };
>
> port@1 {
> - reg = <0>;
> + reg = <1>;
> cluster0_funnel_in_port0: endpoint {
> - slave-mode;
> + direction = <0>;
> remote-endpoint = <&cluster0_etm0_out_port>;
> + coresight,hwid = <0>;
> };
> };
>
> port@2 {
> - reg = <1>;
> + reg = <2>;
> cluster0_funnel_in_port1: endpoint {
> - slave-mode;
> + direction = <0>;
> remote-endpoint = <&cluster0_etm1_out_port>;
> + coresight,hwid = <1>;
> };
> };
> };
> @@ -299,6 +316,8 @@
> port {
> cluster0_etm1_out_port: endpoint {
> remote-endpoint = <&cluster0_funnel_in_port1>;
> + coresight,hwid = <0>;
> + direction = <1>;
> };
> };
> };
> @@ -322,6 +341,8 @@
> port {
> cluster1_etm0_out_port: endpoint {
> remote-endpoint = <&cluster1_funnel_in_port0>;
> + coresight,hwid = <0>;
> + direction = <1>;
> };
> };
> };
> @@ -341,36 +362,42 @@
> reg = <0>;
> cluster1_funnel_out_port: endpoint {
> remote-endpoint = <&main_funnel_in_port1>;
> + coresight,hwid = <0>;
> + direction = <1>;
> };
> };
>
> port@1 {
> - reg = <0>;
> + reg = <1>;
> cluster1_funnel_in_port0: endpoint {
> - slave-mode;
> + direction = <0>;
> remote-endpoint = <&cluster1_etm0_out_port>;
> + coresight,hwid = <0>;
> };
> };
>
> port@2 {
> - reg = <1>;
> + reg = <2>;
> cluster1_funnel_in_port1: endpoint {
> - slave-mode;
> + direction = <0>;
> remote-endpoint = <&cluster1_etm1_out_port>;
> + coresight,hwid = <1>;
> };
> };
> port@3 {
> - reg = <2>;
> + reg = <3>;
> cluster1_funnel_in_port2: endpoint {
> - slave-mode;
> + direction = <0>;
> remote-endpoint = <&cluster1_etm2_out_port>;
> + coresight,hwid = <2>;
> };
> };
> port@4 {
> - reg = <3>;
> + reg = <4>;
> cluster1_funnel_in_port3: endpoint {
> - slave-mode;
> + direction = <0>;
> remote-endpoint = <&cluster1_etm3_out_port>;
> + coresight,hwid = <3>;
> };
> };
> };
> @@ -395,6 +422,8 @@
> port {
> cluster1_etm1_out_port: endpoint {
> remote-endpoint = <&cluster1_funnel_in_port1>;
> + coresight,hwid = <0>;
> + direction = <1>;
> };
> };
> };
> @@ -418,6 +447,8 @@
> port {
> cluster1_etm2_out_port: endpoint {
> remote-endpoint = <&cluster1_funnel_in_port2>;
> + coresight,hwid = <0>;
> + direction = <1>;
> };
> };
> };
> @@ -441,6 +472,8 @@
> port {
> cluster1_etm3_out_port: endpoint {
> remote-endpoint = <&cluster1_funnel_in_port3>;
> + coresight,hwid = <0>;
> + direction = <1>;
> };
> };
> };
> @@ -462,6 +495,8 @@
> reg = <0>;
> replicator_out_port0: endpoint {
> remote-endpoint = <&tpiu_in_port>;
> + coresight,hwid = <0>;
> + direction = <1>;
> };
> };
>
> @@ -469,14 +504,17 @@
> reg = <1>;
> replicator_out_port1: endpoint {
> remote-endpoint = <&etr_in_port>;
> + coresight,hwid = <1>;
> + direction = <1>;
> };
> };
>
> /* replicator input port */
> port@2 {
> - reg = <0>;
> + reg = <2>;
> replicator_in_port0: endpoint {
> - slave-mode;
> + direction = <0>;
> + coresight,hwid = <0>;
> };
> };
> };
> diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
> index c9236c4..27b8036 100644
> --- a/arch/arm64/boot/dts/arm/juno.dts
> +++ b/arch/arm64/boot/dts/arm/juno.dts
> @@ -260,10 +260,11 @@
> &main_funnel {
> ports {
> port@3 {
> - reg = <2>;
> + reg = <3>;
> main_funnel_in_port2: endpoint {
> - slave-mode;
> + direction = <0>;
> remote-endpoint = <&stm_out_port>;
> + coresight,hwid = <2>;
> };
> };
> };
> --
> 2.7.4
>
^ permalink raw reply
* [PATCH 3/3] arm64: dts: renesas: v3hsk: specify GEther PHY IRQ
From: Sergei Shtylyov @ 2018-06-01 20:47 UTC (permalink / raw)
To: Simon Horman, Rob Herring, Catalin Marinas, Will Deacon,
linux-renesas-soc, devicetree
Cc: Mark Rutland, Magnus Damm, linux-arm-kernel
In-Reply-To: <4acc208e-c593-1e8a-00ca-fc9a5574074e@cogentembedded.com>
Specify GEther PHY IRQ in the V3H Starter Kit board's device tree, now
that we have the GPIO support (previously phylib had to resort to polling).
Based on the original (and large) patch by Vladimir Barinov.
Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
---
arch/arm64/boot/dts/renesas/r8a77980-v3hsk.dts | 2 ++
1 file changed, 2 insertions(+)
Index: renesas/arch/arm64/boot/dts/renesas/r8a77980-v3hsk.dts
===================================================================
--- renesas.orig/arch/arm64/boot/dts/renesas/r8a77980-v3hsk.dts
+++ renesas/arch/arm64/boot/dts/renesas/r8a77980-v3hsk.dts
@@ -48,6 +48,8 @@
phy0: ethernet-phy@0 {
reg = <0>;
+ interrupt-parent = <&gpio4>;
+ interrupts = <23 IRQ_TYPE_LEVEL_LOW>;
};
};
^ permalink raw reply
* [PATCH 2/3] arm64: dts: renesas: condor: specify EtherAVB PHY IRQ
From: Sergei Shtylyov @ 2018-06-01 20:45 UTC (permalink / raw)
To: Simon Horman, Rob Herring, Catalin Marinas, Will Deacon,
linux-renesas-soc, devicetree
Cc: Mark Rutland, Magnus Damm, linux-arm-kernel
In-Reply-To: <4acc208e-c593-1e8a-00ca-fc9a5574074e@cogentembedded.com>
Specify EtherAVB PHY IRQ in the Condor board's device tree, now that
we have the GPIO support (previously phylib had to resort to polling).
Based on the original (and large) patch by Vladimir Barinov.
Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
---
arch/arm64/boot/dts/renesas/r8a77980-condor.dts | 2 ++
1 file changed, 2 insertions(+)
Index: renesas/arch/arm64/boot/dts/renesas/r8a77980-condor.dts
===================================================================
--- renesas.orig/arch/arm64/boot/dts/renesas/r8a77980-condor.dts
+++ renesas/arch/arm64/boot/dts/renesas/r8a77980-condor.dts
@@ -59,6 +59,8 @@
phy0: ethernet-phy@0 {
rxc-skew-ps = <1500>;
reg = <0>;
+ interrupt-parent = <&gpio1>;
+ interrupts = <17 IRQ_TYPE_LEVEL_LOW>;
};
};
^ permalink raw reply
* [PATCH 1/3] arm64: dts: renesas: r8a77980: add GPIO support
From: Sergei Shtylyov @ 2018-06-01 20:44 UTC (permalink / raw)
To: Simon Horman, Rob Herring, Catalin Marinas, Will Deacon,
linux-renesas-soc, devicetree
Cc: Mark Rutland, Magnus Damm, linux-arm-kernel
In-Reply-To: <4acc208e-c593-1e8a-00ca-fc9a5574074e@cogentembedded.com>
Describe all 6 GPIO controllers in the R8A77980 device tree.
Based on the original (and large) patch by Vladimir Barinov.
Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
---
arch/arm64/boot/dts/renesas/r8a77980.dtsi | 90 ++++++++++++++++++++++++++++++
1 file changed, 90 insertions(+)
Index: renesas/arch/arm64/boot/dts/renesas/r8a77980.dtsi
===================================================================
--- renesas.orig/arch/arm64/boot/dts/renesas/r8a77980.dtsi
+++ renesas/arch/arm64/boot/dts/renesas/r8a77980.dtsi
@@ -118,6 +118,96 @@
#size-cells = <2>;
ranges;
+ gpio0: gpio@e6050000 {
+ compatible = "renesas,gpio-r8a77980",
+ "renesas,rcar-gen3-gpio";
+ reg = <0 0xe6050000 0 0x50>;
+ interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
+ #gpio-cells = <2>;
+ gpio-controller;
+ gpio-ranges = <&pfc 0 0 22>;
+ #interrupt-cells = <2>;
+ interrupt-controller;
+ clocks = <&cpg CPG_MOD 912>;
+ power-domains = <&sysc R8A77980_PD_ALWAYS_ON>;
+ resets = <&cpg 912>;
+ };
+
+ gpio1: gpio@e6051000 {
+ compatible = "renesas,gpio-r8a77980",
+ "renesas,rcar-gen3-gpio";
+ reg = <0 0xe6051000 0 0x50>;
+ interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
+ #gpio-cells = <2>;
+ gpio-controller;
+ gpio-ranges = <&pfc 0 32 28>;
+ #interrupt-cells = <2>;
+ interrupt-controller;
+ clocks = <&cpg CPG_MOD 911>;
+ power-domains = <&sysc R8A77980_PD_ALWAYS_ON>;
+ resets = <&cpg 911>;
+ };
+
+ gpio2: gpio@e6052000 {
+ compatible = "renesas,gpio-r8a77980",
+ "renesas,rcar-gen3-gpio";
+ reg = <0 0xe6052000 0 0x50>;
+ interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>;
+ #gpio-cells = <2>;
+ gpio-controller;
+ gpio-ranges = <&pfc 0 64 30>;
+ #interrupt-cells = <2>;
+ interrupt-controller;
+ clocks = <&cpg CPG_MOD 910>;
+ power-domains = <&sysc R8A77980_PD_ALWAYS_ON>;
+ resets = <&cpg 910>;
+ };
+
+ gpio3: gpio@e6053000 {
+ compatible = "renesas,gpio-r8a77980",
+ "renesas,rcar-gen3-gpio";
+ reg = <0 0xe6053000 0 0x50>;
+ interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
+ #gpio-cells = <2>;
+ gpio-controller;
+ gpio-ranges = <&pfc 0 96 17>;
+ #interrupt-cells = <2>;
+ interrupt-controller;
+ clocks = <&cpg CPG_MOD 909>;
+ power-domains = <&sysc R8A77980_PD_ALWAYS_ON>;
+ resets = <&cpg 909>;
+ };
+
+ gpio4: gpio@e6054000 {
+ compatible = "renesas,gpio-r8a77980",
+ "renesas,rcar-gen3-gpio";
+ reg = <0 0xe6054000 0 0x50>;
+ interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
+ #gpio-cells = <2>;
+ gpio-controller;
+ gpio-ranges = <&pfc 0 128 25>;
+ #interrupt-cells = <2>;
+ interrupt-controller;
+ clocks = <&cpg CPG_MOD 908>;
+ power-domains = <&sysc R8A77980_PD_ALWAYS_ON>;
+ resets = <&cpg 908>;
+ };
+
+ gpio5: gpio@e6055000 {
+ compatible = "renesas,gpio-r8a77980",
+ "renesas,rcar-gen3-gpio";
+ reg = <0 0xe6055000 0 0x50>;
+ interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>;
+ #gpio-cells = <2>;
+ gpio-controller;
+ gpio-ranges = <&pfc 0 160 15>;
+ #interrupt-cells = <2>;
+ interrupt-controller;
+ clocks = <&cpg CPG_MOD 907>;
+ power-domains = <&sysc R8A77980_PD_ALWAYS_ON>;
+ resets = <&cpg 907>;
+ };
+
pfc: pin-controller@e6060000 {
compatible = "renesas,pfc-r8a77980";
reg = <0 0xe6060000 0 0x50c>;
^ permalink raw reply
* [PATCH 0/3] Add R8A77980/Condor/V3HSK GPIO support
From: Sergei Shtylyov @ 2018-06-01 20:41 UTC (permalink / raw)
To: Simon Horman, Rob Herring, Catalin Marinas, Will Deacon,
linux-renesas-soc, devicetree
Cc: Mark Rutland, Magnus Damm, linux-arm-kernel
Hello!
Here's the set of 3 patches against Simon Horman's 'renesas.git' repo's
'renesas-devel-20180529-v4.17-rc7' tag plus the I2C patches reposted yesterday.
We're adding the R8A77980 GPIO nodes and then describing the PHY IRQ for the
GEther/EtherAVB devices declared earlier.
[1/3] arm64: dts: renesas: r8a77980: add GPIO support
[2/3] arm64: dts: renesas: condor: specify EtherAVB PHY IRQ
[3/3] arm64: dts: renesas: v3hsk: specify GEther PHY IRQ
WBR, Sergei
^ permalink raw reply
* Re: [RFC PATCH 7/8] dts: coresight: Define new bindings for direction of data flow
From: Mathieu Poirier @ 2018-06-01 20:39 UTC (permalink / raw)
To: Suzuki K Poulose
Cc: linux-arm-kernel, sudeep.holla, robh, mark.rutland, frowand.list,
matt.sealey, charles.garcia-tobin, john.horley, mike.leach,
coresight, linux-kernel, devicetree
In-Reply-To: <1527858967-16047-8-git-send-email-suzuki.poulose@arm.com>
On Fri, Jun 01, 2018 at 02:16:06PM +0100, Suzuki K Poulose wrote:
> So far we have relied on an undocumented property "slave-mode",
> to indicate if the given port is input or not. Since we are
> redefining the coresight bindings, define new property for the
> "direction" of data flow for a given connection endpoint in the
> device.
>
> Each endpoint must define the following property.
>
> - "direction" : 0 => Port is input
> 1 => Port is output
>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
> drivers/hwtracing/coresight/of_coresight.c | 20 ++++++++++++++++----
You haven't documented the binding in bindings/arm/coresight.txt the same way
you did with "coresight,hwid". I'm guessing you simply forgot to do a "git add"
on the file when preparing the patchset.
> 1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/of_coresight.c b/drivers/hwtracing/coresight/of_coresight.c
> index 99d7a9c..63c1668 100644
> --- a/drivers/hwtracing/coresight/of_coresight.c
> +++ b/drivers/hwtracing/coresight/of_coresight.c
> @@ -52,7 +52,19 @@ of_coresight_get_endpoint_device(struct device_node *endpoint)
> endpoint, of_dev_node_match);
> }
>
> -static void of_coresight_get_ports(const struct device_node *node,
> +static bool of_coresight_ep_is_input(struct device *dev, struct device_node *ep_node)
I suggested of_coresight_endpoint_get_port_id() in my review of 6/8. I'm good
with either "ep" or "endpoint", as long as the names are consistent.
> +{
> + u32 dir;
> +
> + if (!of_property_read_u32(ep_node, "direction", &dir))
> + return dir == 0;
> +
> + dev_warn_once(dev, "Missing mandatory \"direction\" property!\n");
> + return of_property_read_bool(ep_node, "slave-mode");
> +}
> +
> +static void of_coresight_get_ports(struct device *dev,
> + const struct device_node *node,
> int *nr_inport, int *nr_outport)
> {
> struct device_node *ep = NULL;
> @@ -63,7 +75,7 @@ static void of_coresight_get_ports(const struct device_node *node,
> if (!ep)
> break;
>
> - if (of_property_read_bool(ep, "slave-mode"))
> + if (of_coresight_ep_is_input(dev, ep))
> in++;
> else
> out++;
> @@ -149,7 +161,7 @@ of_get_coresight_platform_data(struct device *dev,
> pdata->name = dev_name(dev);
>
> /* Get the number of input and output port for this component */
> - of_coresight_get_ports(node, &pdata->nr_inport, &pdata->nr_outport);
> + of_coresight_get_ports(dev, node, &pdata->nr_inport, &pdata->nr_outport);
>
> if (pdata->nr_outport) {
> ret = of_coresight_alloc_memory(dev, pdata);
> @@ -168,7 +180,7 @@ of_get_coresight_platform_data(struct device *dev,
> * No need to deal with input ports, processing for as
> * processing for output ports will deal with them.
> */
> - if (of_find_property(ep, "slave-mode", NULL))
> + if (of_coresight_ep_is_input(dev, ep))
> continue;
>
> outport = of_graph_ep_coresight_get_port_id(dev, ep);
> --
> 2.7.4
>
^ permalink raw reply
* Re: [PATCH v8 2/4] dt-bindings: Add bindings for SPI NAND devices
From: Boris Brezillon @ 2018-06-01 20:32 UTC (permalink / raw)
To: Rob Herring
Cc: Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Pawel Moll, Ian Campbell, Richard Weinberger, Kumar Gala,
linux-spi, Marek Vasut, Mark Brown, Geert Uytterhoeven,
Miquel Raynal, MTD Maling List, Brian Norris, David Woodhouse
In-Reply-To: <CAL_JsqKBn9g95irT71fgwWneLCM0jLAKuSPUkNxA_+73Rm8pVA@mail.gmail.com>
Hi Rob,
On Fri, 1 Jun 2018 14:18:35 -0500
Rob Herring <robh+dt@kernel.org> wrote:
> On Fri, Jun 1, 2018 at 12:09 PM, Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> > Hi Geert,
> >
> > On Fri, 1 Jun 2018 16:42:26 +0200
> > Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >
> >> Hi Boris,
> >>
> >> I became interested after reading the cover letter...
> >>
> >> On Fri, Jun 1, 2018 at 3:13 PM, Boris Brezillon
> >> <boris.brezillon@bootlin.com> wrote:
> >> > Add bindings for SPI NAND chips.
> >> >
> >> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> >>
> >> Thanks for your patch!
> >
> > And thanks for reviewing it ;-).
> >
> >>
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/mtd/spi-nand.txt
> >> > @@ -0,0 +1,27 @@
> >> > +SPI NAND flash
> >> > +
> >> > +Required properties:
> >> > +- compatible: should be "spi-nand"
>
> Seems awfully generic if you expect this alone. No chips with quirks
> yet?
Believe it or not, but it seems that NAND vendors agreed on a common
instruction set, made the READID instruction mandatory, and the 2 bytes
returned by this operation seem to be unique and allow us to reliably
extract information about he SPI NAND chip. I'm not saying this will
keep going like that (I'm pretty sure they'll screw up the READID
detection and start re-using IDs for different NANDs at some point),
but so far it seems to work.
> Is there a standard for detection like jedec?
Not that I know of. Some NANDs embed an ONFi table, which is usually
used on parallel/raw NANDs, but this is not mandatory.
>
> >> > +- reg: should encode the chip-select line used to access the NAND chip
>
> "see spi.txt" should be enough.
Okay.
>
> >> > +
> >> > +Optional properties
> >> > +- spi-max-frequency: maximum frequency of the SPI bus the chip can operate at.
> >> > + This should encode board limitations (i.e. max freq can't
> >> > + be achieved due to crosstalk on IO lines).
> >> > + When unspecified, the driver assumes the chip can run at
> >> > + the max frequency defined in the spec (information
> >> > + extracted chip detection time).
> >>
> >> This is a standard property according to
> >> Documentation/devicetree/bindings/spi/spi-bus.txt. Can't you just refer
> >> to that file, or just omit it, as it applies to all SPI slaves anyway?
> >
> > The thing is, the maximum frequency supported by a SPI NAND is directly
> > encoded in the NAND device ID and can be auto-detected. Why should we
> > define spi-max-frequency in the DT when we can automatically detect
> > this information? The only reason one might want to override
> > spi-max-frequency is when the board design impose such restrictions,
> > hence the precision I give here.
>
> This should always be the case. The operating frequency should be
> min(host max, device max) unless the board has restrictions and needs
> to set spi-max-frequency (and we really should have used
> 'bus-frequency' here).
That was my feeling too.
> No doubt though, that is not what has been
> done.
Definitely not, actually, I'm not even sure that this sort if
negotiation is supported by the framework. Right now, the SPI NAND
driver does not try to set max device freq, but that's something I was
planning to work on at some point, and since bindings are supposed to
be set in stone I thought I would clarify the meaning of
spi-max-frequency for the SPI NAND use case.
>
> >> > +- spi-tx-bus-width: The bus width (number of data wires) that is used for MOSI.
> >> > + Only encodes the board constraints (i.e. when not all IO
> >> > + signals are routed on the board). Device constraints are
> >> > + extracted when detecting the chip, and controller
> >> > + constraints are exposed by the SPI mem controller. If this
> >> > + property is missing that means no constraint at the board
> >> > + level.
> >> > +- spi-rx-bus-width: The bus width (number of data wires) that is used for MISO.
> >> > + Only encodes the board constraints (i.e. when not all IO
> >> > + signals are routed on the board). Device constraints are
> >> > + extracted when detecting the chip, and controller
> >> > + constraints are exposed by the SPI mem controller. If this
> >> > + property is missing that means no constraint at the board
> >> > + level.
> >>
> >> This does not match Documentation/devicetree/bindings/spi/spi-bus.txt,
> >> which says the default is 1.
> >
> > Yes, I know.
>
> Like frequency, this should have been similar. I imagine for the
> common case, the number of host and device lines are 1 so it doesn't
> really apply as this was added later on. Perhaps we can reword the
> common definition to work for both?
Do you have a suggestion? Also, I fear that globally changing the
meaning of these props will make most implementations not compliant
with this new definition.
Regards,
Boris
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply
* Re: [RFC PATCH 6/8] dts: coresight: Clean up the device tree graph bindings
From: Mathieu Poirier @ 2018-06-01 20:26 UTC (permalink / raw)
To: Suzuki K Poulose
Cc: linux-arm-kernel, sudeep.holla, robh, mark.rutland, frowand.list,
matt.sealey, charles.garcia-tobin, john.horley, mike.leach,
coresight, linux-kernel, devicetree
In-Reply-To: <1527858967-16047-7-git-send-email-suzuki.poulose@arm.com>
On Fri, Jun 01, 2018 at 02:16:05PM +0100, Suzuki K Poulose wrote:
> The coresight drivers relied on default bindings for graph
> in DT, while reusing the "reg" field of the "ports" to indicate
> the actual hardware port number for the connections. However,
> with the rules getting stricter w.r.t to the address mismatch
> with the label, it is no longer possible to use the port address
> field for the hardware port number. Hence, we add an explicit
> property to denote the hardware port number, "coresight,hwid"
> which must be specified for each "endpoint".
>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Rob Herring <robh@kernel.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
> .../devicetree/bindings/arm/coresight.txt | 26 +++++++++---
> drivers/hwtracing/coresight/of_coresight.c | 46 ++++++++++++++++------
> 2 files changed, 54 insertions(+), 18 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt
> index bd36e40..385581a 100644
> --- a/Documentation/devicetree/bindings/arm/coresight.txt
> +++ b/Documentation/devicetree/bindings/arm/coresight.txt
> @@ -104,7 +104,11 @@ properties to uniquely identify the connection details.
> "slave-mode"
>
> * Hardware Port number at the component:
> - - The hardware port number is assumed to be the address of the "port" component.
> + - (Obsolete) The hardware port number is assumed to be the address of the "port" component.
> + - Each "endpoint" must define the hardware port of the local end of the
> + connection using the following property:
> + "coresight,hwid" - 32bit integer, hardware port number at the local end.
> +
>
>
> Example:
> @@ -120,6 +124,7 @@ Example:
> etb_in_port: endpoint@0 {
> slave-mode;
> remote-endpoint = <&replicator_out_port0>;
> + coresight,hwid = <0>;
> };
> };
> };
> @@ -134,6 +139,7 @@ Example:
> tpiu_in_port: endpoint@0 {
> slave-mode;
> remote-endpoint = <&replicator_out_port1>;
> + coresight,hwid = <0>;
> };
> };
> };
> @@ -154,6 +160,7 @@ Example:
> reg = <0>;
> replicator_out_port0: endpoint {
> remote-endpoint = <&etb_in_port>;
> + coresight,hwid = <0>;
> };
> };
>
> @@ -161,15 +168,17 @@ Example:
> reg = <1>;
> replicator_out_port1: endpoint {
> remote-endpoint = <&tpiu_in_port>;
> + coresight,hwid = <1>;
> };
> };
>
> /* replicator input port */
> port@2 {
> - reg = <0>;
> + reg = <1>;
> replicator_in_port0: endpoint {
> slave-mode;
> remote-endpoint = <&funnel_out_port0>;
> + coresight,hwid = <0>;
> };
> };
> };
> @@ -191,31 +200,35 @@ Example:
> funnel_out_port0: endpoint {
> remote-endpoint =
> <&replicator_in_port0>;
> + coresight,hwid = <0>;
> };
> };
>
> /* funnel input ports */
> port@1 {
> - reg = <0>;
> + reg = <1>;
> funnel_in_port0: endpoint {
> slave-mode;
> remote-endpoint = <&ptm0_out_port>;
> + coresight,hwid = <0>;
> };
> };
>
> port@2 {
> - reg = <1>;
> + reg = <2>;
> funnel_in_port1: endpoint {
> slave-mode;
> remote-endpoint = <&ptm1_out_port>;
> + coresight,hwid = <1>;
> };
> };
>
> port@3 {
> - reg = <2>;
> + reg = <3>;
> funnel_in_port2: endpoint {
> slave-mode;
> remote-endpoint = <&etm0_out_port>;
> + coresight,hwid = <2>;
> };
> };
>
> @@ -233,6 +246,7 @@ Example:
> port {
> ptm0_out_port: endpoint {
> remote-endpoint = <&funnel_in_port0>;
> + coresight,hwid = <0>;
> };
> };
> };
> @@ -247,6 +261,7 @@ Example:
> port {
> ptm1_out_port: endpoint {
> remote-endpoint = <&funnel_in_port1>;
> + coresight,hwid = <0>;
> };
> };
> };
> @@ -263,6 +278,7 @@ Example:
> port {
> stm_out_port: endpoint {
> remote-endpoint = <&main_funnel_in_port2>;
> + coresight,hwid = <0>;
> };
> };
> };
> diff --git a/drivers/hwtracing/coresight/of_coresight.c b/drivers/hwtracing/coresight/of_coresight.c
> index a3f3416..99d7a9c 100644
> --- a/drivers/hwtracing/coresight/of_coresight.c
> +++ b/drivers/hwtracing/coresight/of_coresight.c
> @@ -105,14 +105,37 @@ int of_coresight_get_cpu(const struct device_node *node)
> }
> EXPORT_SYMBOL_GPL(of_coresight_get_cpu);
>
> +/*
> + * of_graph_ep_coresight_get_port_id : Get the hardware port number for the
> + * given endpoint device node. Prefer the explicit "coresight,hwid" property
> + * over the endpoint register id (obsolete bindings).
> + */
> +static int of_graph_ep_coresight_get_port_id(struct device *dev,
> + struct device_node *ep_node)
static int of_coresight_endpoint_get_port_id(struct device *dev,
struct device_node *ep_node)
I think that makes more sense since this function is only visible in this file.
> +{
> + struct of_endpoint ep;
> + int rc, port_id;
> +
> +
> + if (!of_property_read_u32(ep_node, "coresight,hwid", &port_id))
> + return port_id;
> +
> + rc = of_graph_parse_endpoint(ep_node, &ep);
> + if (rc)
> + return rc;
> + dev_warn_once(dev,
> + "ep%d: Mandatory \"coresight,hwid\" property missing."
> + " DT uses obsolete coresight bindings\n", ep.port);
> + return ep.port;
> +}
> +
> struct coresight_platform_data *
> of_get_coresight_platform_data(struct device *dev,
> const struct device_node *node)
> {
> - int ret = 0;
> + int ret = 0, outport, inport;
> struct coresight_platform_data *pdata;
> struct coresight_connection *conn;
> - struct of_endpoint endpoint, rendpoint;
> struct device *rdev;
> struct device_node *ep = NULL;
> struct device_node *rparent = NULL;
> @@ -148,14 +171,10 @@ of_get_coresight_platform_data(struct device *dev,
> if (of_find_property(ep, "slave-mode", NULL))
> continue;
>
> - /* Get a handle on the local endpoint */
> - ret = of_graph_parse_endpoint(ep, &endpoint);
> -
> - if (ret)
> + outport = of_graph_ep_coresight_get_port_id(dev, ep);
> + if (outport < 0)
> continue;
> -
> - /* The local out port number */
> - conn->outport = endpoint.port;
> + conn->outport = outport;
>
> /*
> * Get a handle on the remote endpoint and the device
> @@ -168,15 +187,16 @@ of_get_coresight_platform_data(struct device *dev,
> if (!rparent)
> continue;
>
> - if (of_graph_parse_endpoint(rep, &rendpoint))
> - continue;
> -
> rdev = of_coresight_get_endpoint_device(rparent);
> if (!rdev)
> return ERR_PTR(-EPROBE_DEFER);
>
> + inport = of_graph_ep_coresight_get_port_id(rdev, rep);
> + if (inport < 0)
> + continue;
> +
> conn->child_name = dev_name(rdev);
> - conn->child_port = rendpoint.port;
> + conn->child_port = inport;
> conn++;
> } while (ep);
> }
> --
> 2.7.4
>
^ permalink raw reply
* [PATCH v2 5/5] venus: register separate driver for firmware device
From: Vikash Garodia @ 2018-06-01 20:26 UTC (permalink / raw)
To: hverkuil, mchehab, robh, mark.rutland, andy.gross,
bjorn.andersson, stanimir.varbanov
Cc: linux-media, linux-kernel, linux-arm-msm, linux-soc, devicetree,
vgarodia, acourbot
In-Reply-To: <1527884768-22392-1-git-send-email-vgarodia@codeaurora.org>
A separate child device is added for video firmware.
This is needed to
[1] configure the firmware context bank with the desired SID.
[2] ensure that the iova for firmware region is from 0x0.
Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
---
.../devicetree/bindings/media/qcom,venus.txt | 8 +++-
drivers/media/platform/qcom/venus/core.c | 48 +++++++++++++++++++---
drivers/media/platform/qcom/venus/firmware.c | 20 ++++++++-
drivers/media/platform/qcom/venus/firmware.h | 2 +
4 files changed, 71 insertions(+), 7 deletions(-)
diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt b/Documentation/devicetree/bindings/media/qcom,venus.txt
index 00d0d1b..701cbe8 100644
--- a/Documentation/devicetree/bindings/media/qcom,venus.txt
+++ b/Documentation/devicetree/bindings/media/qcom,venus.txt
@@ -53,7 +53,7 @@
* Subnodes
The Venus video-codec node must contain two subnodes representing
-video-decoder and video-encoder.
+video-decoder and video-encoder, one optional firmware subnode.
Every of video-encoder or video-decoder subnode should have:
@@ -79,6 +79,8 @@ Every of video-encoder or video-decoder subnode should have:
power domain which is responsible for collapsing
and restoring power to the subcore.
+The firmware sub node must contain the iommus specifiers for ARM9.
+
* An Example
video-codec@1d00000 {
compatible = "qcom,msm8916-venus";
@@ -105,4 +107,8 @@ Every of video-encoder or video-decoder subnode should have:
clock-names = "core";
power-domains = <&mmcc VENUS_CORE1_GDSC>;
};
+ venus-firmware {
+ compatible = "qcom,venus-firmware-no-tz";
+ iommus = <&apps_smmu 0x10b2 0x0>;
+ }
};
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 101612b..5cfb3c2 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -179,6 +179,19 @@ static u32 to_v4l2_codec_type(u32 codec)
}
}
+static int store_firmware_dev(struct device *dev, void *data)
+{
+ struct venus_core *core = data;
+
+ if (!core)
+ return -EINVAL;
+
+ if (of_device_is_compatible(dev->of_node, "qcom,venus-firmware-no-tz"))
+ core->fw.dev = dev;
+
+ return 0;
+}
+
static int venus_enumerate_codecs(struct venus_core *core, u32 type)
{
const struct hfi_inst_ops dummy_ops = {};
@@ -279,6 +292,13 @@ static int venus_probe(struct platform_device *pdev)
if (ret < 0)
goto err_runtime_disable;
+ ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
+ if (ret)
+ goto err_runtime_disable;
+
+ /* Attempt to store firmware device */
+ device_for_each_child(dev, core, store_firmware_dev);
+
ret = venus_boot(core);
if (ret)
goto err_runtime_disable;
@@ -303,10 +323,6 @@ static int venus_probe(struct platform_device *pdev)
if (ret)
goto err_core_deinit;
- ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
- if (ret)
- goto err_dev_unregister;
-
ret = pm_runtime_put_sync(dev);
if (ret)
goto err_dev_unregister;
@@ -483,7 +499,29 @@ static __maybe_unused int venus_runtime_resume(struct device *dev)
.pm = &venus_pm_ops,
},
};
-module_platform_driver(qcom_venus_driver);
+
+static int __init venus_init(void)
+{
+ int ret;
+
+ ret = platform_driver_register(&qcom_video_firmware_driver);
+ if (ret)
+ return ret;
+
+ ret = platform_driver_register(&qcom_venus_driver);
+ if (ret)
+ platform_driver_unregister(&qcom_video_firmware_driver);
+
+ return ret;
+}
+module_init(venus_init);
+
+static void __exit venus_exit(void)
+{
+ platform_driver_unregister(&qcom_venus_driver);
+ platform_driver_unregister(&qcom_video_firmware_driver);
+}
+module_exit(venus_exit);
MODULE_ALIAS("platform:qcom-venus");
MODULE_DESCRIPTION("Qualcomm Venus video encoder and decoder driver");
diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
index 058d544..ed29d10 100644
--- a/drivers/media/platform/qcom/venus/firmware.c
+++ b/drivers/media/platform/qcom/venus/firmware.c
@@ -12,6 +12,7 @@
*
*/
+#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/device.h>
#include <linux/firmware.h>
@@ -124,7 +125,7 @@ static int venus_load_fw(struct device *dev, const char *fwname,
}
if (qcom_scm_is_available())
ret = qcom_mdt_load(dev, mdt, fwname, VENUS_PAS_ID, mem_va,
- *mem_phys, *mem_size);
+ *mem_phys, *mem_size, NULL);
else
ret = qcom_mdt_load_no_init(dev, mdt, fwname, VENUS_PAS_ID,
mem_va, *mem_phys, *mem_size, NULL);
@@ -243,3 +244,20 @@ int venus_shutdown(struct venus_core *core)
return ret;
}
+
+static const struct of_device_id firmware_dt_match[] = {
+ { .compatible = "qcom,venus-firmware-no-tz" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, firmware_dt_match);
+
+struct platform_driver qcom_video_firmware_driver = {
+ .driver = {
+ .name = "qcom-video-firmware",
+ .of_match_table = firmware_dt_match,
+ },
+};
+
+MODULE_ALIAS("platform:qcom-video-firmware");
+MODULE_DESCRIPTION("Qualcomm Venus firmware driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/media/platform/qcom/venus/firmware.h b/drivers/media/platform/qcom/venus/firmware.h
index 67fdd89..23c0409 100644
--- a/drivers/media/platform/qcom/venus/firmware.h
+++ b/drivers/media/platform/qcom/venus/firmware.h
@@ -21,6 +21,8 @@
struct device;
+extern struct platform_driver qcom_video_firmware_driver;
+
int venus_boot(struct venus_core *core);
int venus_shutdown(struct venus_core *core);
int venus_set_hw_state(enum tzbsp_video_state, struct venus_core *core);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related
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