Re: [PATCH] drivers: video: omap2: dss: Use PTR_RET function
On 03/20/2013 06:56 AM, Tomi Valkeinen wrote: > On 2013-03-19 10:03, Alexandru Gheorghiu wrote: >> Use PTR_RET function instead of IS_ERR and PTR_ERR. >> Patch found using coccinelle. >> >> Signed-off-by: Alexandru Gheorghiu >> --- >> drivers/video/omap2/dss/core.c |5 + >> 1 file changed, 1 insertion(+), 4 deletions(-) >> >> diff --git a/drivers/video/omap2/dss/core.c b/drivers/video/omap2/dss/core.c >> index f8779d4..60cc6fe 100644 >> --- a/drivers/video/omap2/dss/core.c >> +++ b/drivers/video/omap2/dss/core.c >> @@ -181,10 +181,7 @@ int dss_debugfs_create_file(const char *name, void >> (*write)(struct seq_file *)) >> d = debugfs_create_file(name, S_IRUGO, dss_debugfs_dir, >> write, &dss_debug_fops); >> >> -if (IS_ERR(d)) >> -return PTR_ERR(d); >> - >> -return 0; >> +return PTR_RET(d); >> } >> #else /* CONFIG_OMAP2_DSS_DEBUGFS */ >> static inline int dss_initialize_debugfs(void) >> > > Thanks. I'll apply to omapdss tree. Is this correct? If debugfs_create_file() returns a valid pointer, then now dss_debugfs_create_file() will return a non-zero value on success. I don't think this is what you want. A similar case came up recently here [1]. Jon [1] http://comments.gmane.org/gmane.linux.kernel/1455141 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers: video: omap2: dss: Use PTR_RET function
On 03/20/2013 11:59 AM, Tomi Valkeinen wrote: > On 2013-03-20 17:44, Jon Hunter wrote: >> >> On 03/20/2013 06:56 AM, Tomi Valkeinen wrote: >>> On 2013-03-19 10:03, Alexandru Gheorghiu wrote: >>>> Use PTR_RET function instead of IS_ERR and PTR_ERR. >>>> Patch found using coccinelle. >>>> >>>> Signed-off-by: Alexandru Gheorghiu >>>> --- >>>> drivers/video/omap2/dss/core.c |5 + >>>> 1 file changed, 1 insertion(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/video/omap2/dss/core.c >>>> b/drivers/video/omap2/dss/core.c >>>> index f8779d4..60cc6fe 100644 >>>> --- a/drivers/video/omap2/dss/core.c >>>> +++ b/drivers/video/omap2/dss/core.c >>>> @@ -181,10 +181,7 @@ int dss_debugfs_create_file(const char *name, void >>>> (*write)(struct seq_file *)) >>>>d = debugfs_create_file(name, S_IRUGO, dss_debugfs_dir, >>>>write, &dss_debug_fops); >>>> >>>> - if (IS_ERR(d)) >>>> - return PTR_ERR(d); >>>> - >>>> - return 0; >>>> + return PTR_RET(d); >>>> } >>>> #else /* CONFIG_OMAP2_DSS_DEBUGFS */ >>>> static inline int dss_initialize_debugfs(void) >>>> >>> >>> Thanks. I'll apply to omapdss tree. >> >> Is this correct? If debugfs_create_file() returns a valid pointer, then >> now dss_debugfs_create_file() will return a non-zero value on success. I >> don't think this is what you want. A similar case came up recently here [1]. > > Hmm. I don't follow. And I don't understand the post where you referred. > > PTR_RET is defined as: > > if (IS_ERR(ptr)) > return PTR_ERR(ptr); > else > return 0; > > How's that different from the original code? Oops sorry, I missed that you were returning PTR_RET() and not PTR_ERR(). Yes that looks fine. Sorry for the noise! Jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers: video: omap2: dss: Use PTR_RET function
On 03/20/2013 01:02 PM, Jon Hunter wrote: > > On 03/20/2013 11:59 AM, Tomi Valkeinen wrote: >> On 2013-03-20 17:44, Jon Hunter wrote: >>> >>> On 03/20/2013 06:56 AM, Tomi Valkeinen wrote: >>>> On 2013-03-19 10:03, Alexandru Gheorghiu wrote: >>>>> Use PTR_RET function instead of IS_ERR and PTR_ERR. >>>>> Patch found using coccinelle. >>>>> >>>>> Signed-off-by: Alexandru Gheorghiu >>>>> --- >>>>> drivers/video/omap2/dss/core.c |5 + >>>>> 1 file changed, 1 insertion(+), 4 deletions(-) >>>>> >>>>> diff --git a/drivers/video/omap2/dss/core.c >>>>> b/drivers/video/omap2/dss/core.c >>>>> index f8779d4..60cc6fe 100644 >>>>> --- a/drivers/video/omap2/dss/core.c >>>>> +++ b/drivers/video/omap2/dss/core.c >>>>> @@ -181,10 +181,7 @@ int dss_debugfs_create_file(const char *name, void >>>>> (*write)(struct seq_file *)) >>>>> d = debugfs_create_file(name, S_IRUGO, dss_debugfs_dir, >>>>> write, &dss_debug_fops); >>>>> >>>>> - if (IS_ERR(d)) >>>>> - return PTR_ERR(d); >>>>> - >>>>> - return 0; >>>>> + return PTR_RET(d); >>>>> } >>>>> #else /* CONFIG_OMAP2_DSS_DEBUGFS */ >>>>> static inline int dss_initialize_debugfs(void) >>>>> >>>> >>>> Thanks. I'll apply to omapdss tree. >>> >>> Is this correct? If debugfs_create_file() returns a valid pointer, then >>> now dss_debugfs_create_file() will return a non-zero value on success. I >>> don't think this is what you want. A similar case came up recently here [1]. >> >> Hmm. I don't follow. And I don't understand the post where you referred. Yes looking at Russell's response I am not sure I following now as it is also using PTR_RET() and not PTR_ERR(). My eyes deceived me on this one. Jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mach_omap2: use PTR_RET instead of IS_ERR + PTR_ERR
On 03/12/2013 06:05 AM, Russell King - ARM Linux wrote:
> On Tue, Mar 12, 2013 at 09:58:29AM +0200, Silviu-Mihai Popescu wrote:
>> This uses PTR_RET instead of IS_ERR and PTR_ERR in order to increase
>> readability.
>>
>> Signed-off-by: Silviu-Mihai Popescu
>> ---
>> arch/arm/mach-omap2/devices.c |4 ++--
>> arch/arm/mach-omap2/fb.c |5 +
>> arch/arm/mach-omap2/gpmc.c|2 +-
>> arch/arm/mach-omap2/pmu.c |5 +
>> 4 files changed, 5 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
>> index 1ec7f05..2a0816e 100644
>> --- a/arch/arm/mach-omap2/devices.c
>> +++ b/arch/arm/mach-omap2/devices.c
>> @@ -66,7 +66,7 @@ static int __init omap3_l3_init(void)
>>
>> WARN(IS_ERR(pdev), "could not build omap_device for %s\n", oh_name);
>>
>> -return IS_ERR(pdev) ? PTR_ERR(pdev) : 0;
>> +return PTR_RET(pdev);
>
> This is incorrect.
>
> The return value will be tested for < 0. Kernel pointers in general are
> all above 3GB, and so are all "< 0".
>
> I'm afraid none of these changes stuff is an improvement - they all
> introduce bugs.
Sorry I am now not sure I follow you here. Someone just pointed out to
me that PTR_RET() is defined as ...
static inline int __must_check PTR_RET(const void *ptr)
{
if (IS_ERR(ptr))
return PTR_ERR(ptr);
else
return 0;
}
So the above change appears to be equivalent. Is there something that is
wrong with the current implementation that needs to be fixed?
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 09/11] mfd: twl-core: Collect global variables behind one private structure (global)
On 01/16/2013 07:53 AM, Peter Ujfalusi wrote:
> Gather the global variables under a single structure and allocate it with
> devm_kzalloc(). It is easier to see them and if in the future we try to add
> support for multiple instance of twl in the system it is going to be much
> simpler.
>
> Signed-off-by: Peter Ujfalusi
> ---
> drivers/mfd/twl-core.c | 104
> +++--
> 1 file changed, 57 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
> index 1827088..e2895a4 100644
> --- a/drivers/mfd/twl-core.c
> +++ b/drivers/mfd/twl-core.c
> @@ -141,33 +141,28 @@
>
> /*--*/
>
> -/* is driver active, bound to a chip? */
> -static bool inuse;
> -
> -/* TWL IDCODE Register value */
> -static u32 twl_idcode;
> -
> -static unsigned int twl_id;
> -unsigned int twl_rev(void)
> -{
> - return twl_id;
> -}
> -EXPORT_SYMBOL(twl_rev);
> -
> /* Structure for each TWL4030/TWL6030 Slave */
> struct twl_client {
> struct i2c_client *client;
> struct regmap *regmap;
> };
>
> -static struct twl_client *twl_modules;
> -
> /* mapping the module id to slave id and base address */
> struct twl_mapping {
> unsigned char sid; /* Slave ID */
> unsigned char base; /* base address */
> };
> -static struct twl_mapping *twl_map;
> +
> +struct twl_private {
> + bool ready; /* The core driver is ready to be used */
> + u32 twl_idcode; /* TWL IDCODE Register value */
> + unsigned int twl_id;
> +
> + struct twl_mapping *twl_map;
> + struct twl_client *twl_modules;
> +};
> +
> +static struct twl_private *twl_priv;
>
> static struct twl_mapping twl4030_map[] = {
> /*
> @@ -300,6 +295,12 @@ static inline int twl_get_last_module(void)
>
> /* Exported Functions */
>
> +unsigned int twl_rev(void)
> +{
> + return twl_priv ? twl_priv->twl_id : 0;
> +}
> +EXPORT_SYMBOL(twl_rev);
> +
> /**
> * twl_i2c_write - Writes a n bit register in TWL4030/TWL5030/TWL60X0
> * @mod_no: module number
> @@ -322,16 +323,17 @@ int twl_i2c_write(u8 mod_no, u8 *value, u8 reg,
> unsigned num_bytes)
> pr_err("%s: invalid module number %d\n", DRIVER_NAME, mod_no);
> return -EPERM;
> }
> - if (unlikely(!inuse)) {
> + if (unlikely(!twl_priv->ready)) {
This is causing the kernel to panic on all my omap2 boards when booting
linux-next
because twl_priv is not initialised yet.
> pr_err("%s: not initialized\n", DRIVER_NAME);
> return -EPERM;
> }
>
> - sid = twl_map[mod_no].sid;
> - twl = &twl_modules[sid];
> + sid = twl_priv->twl_map[mod_no].sid;
> + twl = &twl_priv->twl_modules[sid];
>
> - ret = regmap_bulk_write(twl->regmap, twl_map[mod_no].base + reg,
> - value, num_bytes);
> + ret = regmap_bulk_write(twl->regmap,
> + twl_priv->twl_map[mod_no].base + reg, value,
> + num_bytes);
>
> if (ret)
> pr_err("%s: Write failed (mod %d, reg 0x%02x count %d)\n",
> @@ -360,16 +362,17 @@ int twl_i2c_read(u8 mod_no, u8 *value, u8 reg, unsigned
> num_bytes)
> pr_err("%s: invalid module number %d\n", DRIVER_NAME, mod_no);
> return -EPERM;
> }
> - if (unlikely(!inuse)) {
> + if (unlikely(!twl_priv->ready)) {
Same problem here.
Here is a fix ...
>From 141fcbbdee6bdc14d5a444ff20fad6b3440215dc Mon Sep 17 00:00:00 2001
From: Jon Hunter
Date: Fri, 8 Feb 2013 12:42:20 -0600
Subject: [PATCH] ARM: OMAP2+: Fix kernel panic on boot
Commit 8a6aaa3 (mfd: twl-core: Collect global variables behind one
private structure (global)) removed the variable "inuse" that is used
to determine if the device has been initialised and now use the
twl_priv structure instead. This is causing the kernel to panic on all
OMAP2+ devices, because we try to access the twl_priv->ready member
before checking if twl_priv is initialised. Fix this and move this test
to the beginning of the twl_i2c_read/write function because
twl_get_last_module() also uses the twl_priv structure.
Signed-off-by: Jon Hunter
---
drivers/mfd/twl-core.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
index 557f9ee..89ab4d9 100644
--- a/drivers/mfd/twl-core.c
+++ b/drivers/mfd/twl-core.c
@@ -316,12 +316,12 @@ int twl_i2c_write(u8 mod_no, u8 *value, u8 reg, uns
Re: [PATCH 1/1] ARM: OMAP4: Add OMAP4 Blaze Tablet support
On 02/09/2013 08:52 PM, Tony Lindgren wrote: > Hi, > > * Ruslan Bilovol [130208 11:41]: >> The OMAP4 Blaze Tablet is TI OMAP4 processor-based >> development platform in a tablet formfactor. >> The platform contains many of the features found in >> present-day handsets (such as audio, video, wireless >> functions and user interfaces) and in addition >> contains features for software development and test. >> >> This patch adds initial support for the OMAP4 Blaze >> Tablet development platform. Additional functionality >> depends on different drivers and code modifications that >> are not upstreamed yet so will be added later. > > Nice that you have it working, however, we're not adding > any more board-*.c files as we're moving things to device > tree based booting. > > Care to try to get the basic .dts file done for this? > You will probably need to add CONFIG_ARM_APPENDED_DTB=y > and CONFIG_ARM_ATAG_DTB_COMPAT=y to omap2plus_defconfig > and append the .dtb file to zImage and run mkimage then > manually to boot it. But you should get serial and MMC > at least working to start with :) Please note that the blaze is derived from the omap4-sdp board and so I would hope that we can use the existing for sdp dts and board file for blaze. In fact this is what I do today for basic booting. So unless there is some feature on the blaze that is not compatible with the original sdp, we should just use the sdp dts. Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 09/11] mfd: twl-core: Collect global variables behind one private structure (global)
On 02/08/2013 11:50 PM, Peter Ujfalusi wrote:
> On 02/08/2013 07:56 PM, Jon Hunter wrote:
>>> /**
>>> * twl_i2c_write - Writes a n bit register in TWL4030/TWL5030/TWL60X0
>>> * @mod_no: module number
>>> @@ -322,16 +323,17 @@ int twl_i2c_write(u8 mod_no, u8 *value, u8 reg,
>>> unsigned num_bytes)
>>> pr_err("%s: invalid module number %d\n", DRIVER_NAME, mod_no);
>>> return -EPERM;
>>> }
>>> - if (unlikely(!inuse)) {
>>> + if (unlikely(!twl_priv->ready)) {
>>
>> This is causing the kernel to panic on all my omap2 boards when booting
>> linux-next
>> because twl_priv is not initialised yet.
>
> Good catch.
> I just wonder from where the twl_* call is coming on OMAP2. AFAIK the twl code
> is for OMAP3/4, for OMAP2 Menelaus is the one which is used.
> I'm currently working on to remove all those twl_* calls from random places in
> the kernel so we will only access twl via the MFD stack.
Good point. I just noticed that none of my omap2+ board were booting and
on omap3/4 I was the panic in the twl code. I can't say that I checked
the panic on omap2, so may be that was another problem?
I will update the changelog and re-send the patch.
Cheers
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 09/11] mfd: twl-core: Collect global variables behind one private structure (global)
On 02/12/2013 01:26 AM, Peter Ujfalusi wrote: > On 02/11/2013 09:22 PM, Jon Hunter wrote: >> Good point. I just noticed that none of my omap2+ board were booting and >> on omap3/4 I was the panic in the twl code. I can't say that I checked >> the panic on omap2, so may be that was another problem? > > Do you have insights on the code path leading to a crash on OMAP3/4? I have > been running this code on several boards (BeagleBoard, Zoom2, PandaBoard, > Blaze) and have not seen a crash. Here is the panic log ... [2.286132] Unable to handle kernel NULL pointer dereference at virtual address [2.294738] pgd = c0004000 [2.297576] [] *pgd= [2.301361] Internal error: Oops: 5 [#1] SMP ARM [2.306243] Modules linked in: [2.309448] CPU: 0Not tainted (3.8.0-rc6-next-20130207-00016-g735c237 #35) [2.317169] PC is at twl_i2c_read+0x3c/0xec [2.321563] LR is at twl_i2c_read+0x1c/0xec [2.325988] pc : []lr : []psr: 8153 [2.325988] sp : c702fed0 ip : fp : [2.338043] r10: c702e000 r9 : c06e84e8 r8 : c06e51c8 [2.343536] r7 : 0001 r6 : 0006 r5 : c702fef6 r4 : 0004 [2.350402] r3 : c129508c r2 : 0006 r1 : c702fef6 r0 : 000e [2.357269] Flags: Nzcv IRQs on FIQs off Mode SVC_32 ISA ARM Segment kernel [2.365051] Control: 10c5387d Table: 80004019 DAC: 0017 [2.371093] Process swapper/0 (pid: 1, stack limit = 0xc702e240) [2.377410] Stack: (0xc702fed0 to 0xc703) [2.382019] fec0: c0d42180 c0d429d0 c70a7640 c07354c4 [2.390624] fee0: 0001 c0719798 c0d42180 c06f2cc0 c04e76cc c06ee1ac c07354c4 [2.399230] ff00: 0007 c06f2d64 0017 c06fb308 c06f07a4 c0cb8580 c06edee0 [2.407836] ff20: c06eded4 c06e8504 c0d42180 c0008768 009e c00611b4 0001 [2.416442] ff40: c061994c c06b7548 0007 0007 c07354c4 0007 c0719798 [2.425048] ff60: c0d42180 c06e51c8 c07197a0 009e c06e590c 0007 0007 [2.433654] ff80: c06e51c8 c04d26ec [2.442260] ffa0: c04d26f4 c00137b0 [2.450866] ffc0: [2.459472] ffe0: 0013 80fb6c10 71bbcd20 [2.468078] [] (twl_i2c_read+0x3c/0xec) from [] (omap3_twl_set_sr_bit+0x3c/0xb4) [2.477722] [] (omap3_twl_set_sr_bit+0x3c/0xb4) from [] (omap3_twl_init+0x2c/0x70) [2.487518] [] (omap3_twl_init+0x2c/0x70) from [] (omap_pmic_late_init+0x18/0x24) [2.497222] [] (omap_pmic_late_init+0x18/0x24) from [] (omap2_common_pm_late_init+0x18/0xd0) [2.507934] [] (omap2_common_pm_late_init+0x18/0xd0) from [] (omap3_init_late+0xc/0x18) [2.518188] [] (omap3_init_late+0xc/0x18) from [] (init_machine_late+0x1c/0x28) [2.527740] [] (init_machine_late+0x1c/0x28) from [] (do_one_initcall+0x100/0x16c) [2.537536] [] (do_one_initcall+0x100/0x16c) from [] (kernel_init_freeable+0xfc/0x1c8) [2.547698] [] (kernel_init_freeable+0xfc/0x1c8) from [] (kernel_init+0x8/0xe4) [2.557250] [] (kernel_init+0x8/0xe4) from [] (ret_from_fork+0x14/0x24) > But the fix is valid. Thanks. I saw this on linux-next earlier this week, but now I am not seeing it and the fix I posted is not there. So I am not sure what changed to cause this to occur earlier this week but the problem appears to be gone again. However, at least the fix will prevent such panics if someone is calling twl_i2c_read/write too early. Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/4] ARM: dts: omap5: Update GPIO with address space and interrupts
Hi Seb,
On 10/23/2012 03:37 AM, Sebastien Guiriec wrote:
> Add base address and interrupt line inside Device Tree data for
> OMAP5
>
> Signed-off-by: Sebastien Guiriec
> ---
> arch/arm/boot/dts/omap5.dtsi | 16
> 1 file changed, 16 insertions(+)
>
> diff --git a/arch/arm/boot/dts/omap5.dtsi b/arch/arm/boot/dts/omap5.dtsi
> index 42c78be..9e39f9f 100644
> --- a/arch/arm/boot/dts/omap5.dtsi
> +++ b/arch/arm/boot/dts/omap5.dtsi
> @@ -104,6 +104,8 @@
>
> gpio1: gpio@4ae1 {
> compatible = "ti,omap4-gpio";
> + reg = <0x4ae1 0x200>;
> + interrupts = <0 29 0x4>;
> ti,hwmods = "gpio1";
> gpio-controller;
> #gpio-cells = <2>;
I am wondering if we should add the "interrupt-parent" property to add
nodes in the device-tree source. I know that today the interrupt-parent
is being defined globally, but when device-tree maps an interrupt for a
device it searches for the interrupt-parent starting the current device
node.
So in other words, for gpio1 it will search the gpio1 binding for
"interrupt-parent" and if not found move up a level and search again. It
will keep doing this until it finds the "interrupt-parent".
Therefore, I believe it will improve search time and hence, boot time if
we have interrupt-parent defined in each node.
Cheers
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/4] ARM: dts: omap5: Update GPIO with address space and interrupts
On 10/23/2012 10:09 AM, Benoit Cousson wrote:
> On 10/23/2012 04:49 PM, Jon Hunter wrote:
>> Hi Seb,
>>
>> On 10/23/2012 03:37 AM, Sebastien Guiriec wrote:
>>> Add base address and interrupt line inside Device Tree data for
>>> OMAP5
>>>
>>> Signed-off-by: Sebastien Guiriec
>>> ---
>>> arch/arm/boot/dts/omap5.dtsi | 16
>>> 1 file changed, 16 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/omap5.dtsi b/arch/arm/boot/dts/omap5.dtsi
>>> index 42c78be..9e39f9f 100644
>>> --- a/arch/arm/boot/dts/omap5.dtsi
>>> +++ b/arch/arm/boot/dts/omap5.dtsi
>>> @@ -104,6 +104,8 @@
>>>
>>> gpio1: gpio@4ae1 {
>>> compatible = "ti,omap4-gpio";
>>> + reg = <0x4ae1 0x200>;
>>> + interrupts = <0 29 0x4>;
>>> ti,hwmods = "gpio1";
>>> gpio-controller;
>>> #gpio-cells = <2>;
>>
>> I am wondering if we should add the "interrupt-parent" property to add
>> nodes in the device-tree source. I know that today the interrupt-parent
>> is being defined globally, but when device-tree maps an interrupt for a
>> device it searches for the interrupt-parent starting the current device
>> node.
>>
>> So in other words, for gpio1 it will search the gpio1 binding for
>> "interrupt-parent" and if not found move up a level and search again. It
>> will keep doing this until it finds the "interrupt-parent".
>>
>> Therefore, I believe it will improve search time and hence, boot time if
>> we have interrupt-parent defined in each node.
>
> Mmm, I'm not that sure. it will increase the size of the blob, so
> increase the time to load it and then to parse it. Where in the current
> case, it is just going up to the parent node using the already
> un-flatten tree in memory and thus that should not take that much time.
Yes it will definitely increase the size, so that could slow things down.
> That being said, it might be interesting to benchmark that to see what
> is the real impact.
Right, I wonder what the key functions are we need to benchmark to get
an overall feel for what is best? Right now I am seeing some people add
the interrupt-parent for device nodes and others not. Ideally we should
be consistent, but at the same time it is probably something that we can
easily sort out later. So not a big deal either way.
Cheers
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/4] ARM: dts: omap5: Update GPIO with address space and interrupts
Hi Mitch, On 10/23/2012 11:55 AM, Mitch Bradley wrote: > On 10/23/2012 4:49 AM, Jon Hunter wrote: > >> Therefore, I believe it will improve search time and hence, boot time if >> we have interrupt-parent defined in each node. > > I strongly suspect (based on many years of performance tuning, with > special focus on boot time) that the time difference will be completely > insignificant. The total extra time for walking up the interrupt tree > for every interrupt in a large system is comparable to the time it takes > to send a few characters out a UART. So you can get more improvement > from eliminating a single printk() than from globally adding per-node > interrupt-parent. > > Furthermore, the cost of processing all of the interrupt-parent > properties is probably similar to the cost of the avoided tree walks. > > CPU cycles are very fast compared to I/O register accesses, say a factor > of 100. Now consider that many modern devices contain embedded > microcontrollers (SD cards, network interface modules, USB hubs and > devices, ...), and those devices usually require various delays measured > in milliseconds, to ensure that the microcontroller is ready for the > next initialization step. Those delays are extremely long compared to > CPU cycles. Obviously, some of that can be overlapped by careful > multithreading, but that isn't free either. > > The bottom line is that I'm pretty sure that adding per-node > interrupt-parent would not be worthwhile from the standpoint of speeding > up boot time. Absolutely, I don't expect this to miraculously improve the boot time or suggest that this is a major contributor to boot time, but what is the best approach in general in terms of efficiency (memory and time). In other words, is there a best practice? And from your feedback, I understand that adding a global interrupt-parent is a good practice. For a bit of fun, I took an omap4430 board and benchmarked the time taken by the of_irq_find_parent() when interrupt-parent was defined for each node using interrupts and without. There were a total of 47 device nodes using interrupts. Adding the interrupt-parent to all 47 nodes increased the dtb from 13211 bytes to 13963 bytes. On boot-up I saw 117 calls to of_irq_find_parent() for this platform (there appears to be multiple calls for a given device). Without interrupt-parent defined for each node total time spent in of_irq_find_parent() was 1.028 ms where as with interrupt-parent defined for each node the total time was 0.4032 ms. This was done using a 38.4MHz timer and the overhead of reading the timer 117 times was about 36 us. I understand that this does not provide the full picture, but I wanted to get a better handle on the times here. So yes the overall overhead here is not significant for us to worry about. Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: discrepancy while save and restore of debounce registers
Hi Gururaja,
On 10/17/2012 01:13 AM, Hebbar, Gururaja wrote:
> Hi,
>
> I came across a peculiar issue while updating GPIO debounce registers on
> OMAP platform.
>
> According to mainline commit ae547354a8ed59f19b57f7e1de9c7816edfc3537
>
> gpio/omap: save and restore debounce registers
>
> GPIO debounce registers need to be saved and restored for proper functioning
> of driver.
>
> ...
> @@ -1363,6 +1369,12 @@ static void omap_gpio_restore_context(struct gpio_bank
> *bank)
> __raw_writel(bank->context.fallingdetect,
> bank->base + bank->regs->fallingdetect);
> __raw_writel(bank->context.dataout, bank->base + bank->regs->dataout);
> + if (bank->dbck_enable_mask) {
> + __raw_writel(bank->context.debounce, bank->base +
> + bank->regs->debounce);
> + __raw_writel(bank->context.debounce_en,
> + bank->base + bank->regs->debounce_en);
> + }
> }
>
>
> Due to copy/paste of this commit into my local tree, I missed the check for
> bank->dbck_enable_mask, and directly restored the saved value from context.
>
> After this, I saw random crashes when accessing different registers (sometimes
> its OE register and sometime its DATAOUT register).
>
> These crashes were seen across 2nd and subsequent suspend/resume.
>
> My doubt/questions are
> 1. Why should debounce registers be updated only when it's accessed
> previously?
If debounce is not being used by any of the gpios, then there is no need
to restore them as there are no bits set. So this makes sense and saves
a couple register writes.
> 2. What is the relation between updating debounce registers and crash seen on
> others registers?
This I am not sure about. I gave this a quick try on my omap3430 beagle
board, but I did not see any side-effects from doing this. However, if
you are always restoring the debounce context regardless of whether
debounce is being used, then you could be writing bad values to the
debounce registers as the context variables bank->context.debouce and
bank->context.debouce_en may not initialised. So that is bad. However,
that said I am still not sure how this could cause a crash.
Can you share more details on ...
1. The OMAP platform you are using?
2. What linux distro/environment you are using?
3. If there are any specific steps to reproduce this 100% of the time?
Cheers
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: discrepancy while save and restore of debounce registers
Hi Gururaja, On 10/18/2012 12:31 AM, Hebbar, Gururaja wrote: > Jon, > > On Thu, Oct 18, 2012 at 02:42:01, Hunter, Jon wrote: [snip] >>> My doubt/questions are >>> 1. Why should debounce registers be updated only when it's accessed >>> previously? >> >> If debounce is not being used by any of the gpios, then there is no need >> to restore them as there are no bits set. So this makes sense and saves >> a couple register writes. > > What I want to know is that other than saving register writes, is there any > other important stuff that specifies this requirement. >From a HW perspective, none that I can see. >From a SW perspective, yes, as I mentioned if you restore these registers without first initialising the context variables where these registers are stored, then you may be restoring unknown values to the registers and that is bad. >>> 2. What is the relation between updating debounce registers and crash seen >>> on >>> others registers? >> >> This I am not sure about. I gave this a quick try on my omap3430 beagle >> board, but I did not see any side-effects from doing this. However, if >> you are always restoring the debounce context regardless of whether >> debounce is being used, then you could be writing bad values to the >> debounce registers as the context variables bank->context.debouce and >> bank->context.debouce_en may not initialised. So that is bad. However, >> that said I am still not sure how this could cause a crash. >> >> Can you share more details on ... > > Sorry for missing below details in first post. > >> 1. The OMAP platform you are using? > > I was trying this on TI AM335x platform (repo below). On AM335x EVM board > > http://arago-project.org/git/projects/?p=linux-am33x.git;a=shortlog; > h=refs/heads/v3.2-staging > >> 2. What linux distro/environment you are using? > > Arago AM335x PSP release (linux 3.2 + am335x patch-set) > >> 3. If there are any specific steps to reproduce this 100% of the time? > > On top of this tree, try suspend/resume using "echo mem > /syspower/state" I have a beagle-bone but unfortunately, suspend does not appear to be supported in the mainline kernel yet so I am unable to test this on the am335x on the mainline. Have you compared the gpio driver from your v3.2 branch with the current mainline to see how different this code is? Ideally, it would be good to test on the mainline kernel for another data point to see if this is local to your branch. Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: CoreSight framework and drivers
On 12/21/2012 04:18 PM, Pratik Patel wrote: > On Thu, Dec 20, 2012 at 04:54:38PM -0600, Jon Hunter wrote: >> >> On 12/20/2012 01:51 PM, Pratik Patel wrote: >>> On Thu, Dec 20, 2012 at 11:46:13AM -0600, Jon Hunter wrote: >>>> >>>> On 12/19/2012 03:24 PM, Pratik Patel wrote: >>>> >>>> [snip] >>>> >>>>> Currently we use the CoreSight virtual bus to conveniently list >>>>> sysfs configuration attributes for all the registered CoreSight >>>>> devices. >>>>> >>>>> For eg: >>>>> /sys/bus/coresight/devices/coresight-etm0/ >>>>> /sys/bus/coresight/devices/coresight-etm1/ >>>>> /sys/bus/coresight/devices/coresight-stm/ >>>>> /sys/bus/coresight/devices/coresight-tmc-etf/ >>>>> ... >>>>> ... >>>>> >>>>> Some of the attributes are based on device type (i.e. source, >>>>> link or sink) so they will exist for all devices of that type >>>>> while some are device specific. >>>>> >>>>> Maybe I am misunderstanding the question but are you suggesting >>>>> to register CoreSight devices to the AMBA bus instead of the >>>>> CoreSight core layer code? >>>> >>>> Yes exactly. >>>> >>>>> Will Deacon mentioned earlier that AMBA framework can be changed >>>>> to accomodate devices with a different class but I am more >>>>> concerned with losing some of the stuff that the core layer code >>>>> does (eg. coresight_register, coresight_enable, coresight_disable >>>>> in coresight.c) if we register CoreSight drivers to the AMBA bus >>>>> without letting the core layer know about the device connections. >>>> >>>> I may be missing something, but couldn't you keep all the >>>> register/enable/disable stuff but just register the device with the amba >>>> bus? Obviously some changes would need to be made. >>>> >>> >>> Ok, so are you referring to making CoreSight devices register >>> with AMBA bus instead of platform bus keeping everything else >>> intact? >> >> Yes exactly. However, please note I am not saying that we should do >> this, and I asking what direction does the community want us to take >> here? Platform bus or AMBA bus? >> >>>> Personally, I don't have strong feeling either way, but we have ETM/ETB >>>> drivers using AMBA today and so I am hoping we can come to agreement on >>>> this going forward. >>>> >>>> Russell, Will, what are your thoughts? >>>> >>>> Otherwise, looking at the code, I like what you have implemented. I >>>> still need to look closer, but I am struggling to figure out how a >>>> coresight device such as the cross-trigger-interface fits with this >>>> model. This model appears to be geared towards coresight devices used >>>> for traces purposes and are either source, links or sinks. The >>>> cross-trigger-interface is not a source or a sink. However, although you >>>> it could be considered as a link (routing events), it is not really, as >>>> it may not link to other coresight sinks/source. >>>> >>>> In my case, I have PMU-IRQ --> CTI --> GIC. So a non-coresight source >>>> and sink. In away the CTI looks more like a 2nd-level interrupt >>>> controller than anything else. Hence, another type of coresight device >>>> may be needed in addition to source, links and sinks. Or link is >>>> extended in some way to connect to non-coresight sources/sinks. >>>> >>>> Let me know if you have any thoughts. >>>> >>> >>> I had left the "None" type for miscellaneous stuff but I agree it >>> should be a separate type - maybe "debug". >>> >>> Having said that I like the CTI driver you have uploaded. Need to >>> look at it in more detail. Since CTI connections can vary between >>> chip to chip, we need a generic way to deal with it. >> >> Yes if you have any ideas let me know. As Will had mentioned it would be >> good to have a common function table all these devices could use too. I >> will take a closer look at what you have. >> > > I had started on a CTI driver much in line with your current > implementation but your approach looks good to me. > > Looking at the code though, I couldn't find a way
Re: CoreSight framework and drivers
On 12/19/2012 03:24 PM, Pratik Patel wrote: [snip] > Currently we use the CoreSight virtual bus to conveniently list > sysfs configuration attributes for all the registered CoreSight > devices. > > For eg: > /sys/bus/coresight/devices/coresight-etm0/ > /sys/bus/coresight/devices/coresight-etm1/ > /sys/bus/coresight/devices/coresight-stm/ > /sys/bus/coresight/devices/coresight-tmc-etf/ > ... > ... > > Some of the attributes are based on device type (i.e. source, > link or sink) so they will exist for all devices of that type > while some are device specific. > > Maybe I am misunderstanding the question but are you suggesting > to register CoreSight devices to the AMBA bus instead of the > CoreSight core layer code? Yes exactly. > Will Deacon mentioned earlier that AMBA framework can be changed > to accomodate devices with a different class but I am more > concerned with losing some of the stuff that the core layer code > does (eg. coresight_register, coresight_enable, coresight_disable > in coresight.c) if we register CoreSight drivers to the AMBA bus > without letting the core layer know about the device connections. I may be missing something, but couldn't you keep all the register/enable/disable stuff but just register the device with the amba bus? Obviously some changes would need to be made. Personally, I don't have strong feeling either way, but we have ETM/ETB drivers using AMBA today and so I am hoping we can come to agreement on this going forward. Russell, Will, what are your thoughts? Otherwise, looking at the code, I like what you have implemented. I still need to look closer, but I am struggling to figure out how a coresight device such as the cross-trigger-interface fits with this model. This model appears to be geared towards coresight devices used for traces purposes and are either source, links or sinks. The cross-trigger-interface is not a source or a sink. However, although you it could be considered as a link (routing events), it is not really, as it may not link to other coresight sinks/source. In my case, I have PMU-IRQ --> CTI --> GIC. So a non-coresight source and sink. In away the CTI looks more like a 2nd-level interrupt controller than anything else. Hence, another type of coresight device may be needed in addition to source, links and sinks. Or link is extended in some way to connect to non-coresight sources/sinks. Let me know if you have any thoughts. Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: CoreSight framework and drivers
On 12/20/2012 01:51 PM, Pratik Patel wrote: > On Thu, Dec 20, 2012 at 11:46:13AM -0600, Jon Hunter wrote: >> >> On 12/19/2012 03:24 PM, Pratik Patel wrote: >> >> [snip] >> >>> Currently we use the CoreSight virtual bus to conveniently list >>> sysfs configuration attributes for all the registered CoreSight >>> devices. >>> >>> For eg: >>> /sys/bus/coresight/devices/coresight-etm0/ >>> /sys/bus/coresight/devices/coresight-etm1/ >>> /sys/bus/coresight/devices/coresight-stm/ >>> /sys/bus/coresight/devices/coresight-tmc-etf/ >>> ... >>> ... >>> >>> Some of the attributes are based on device type (i.e. source, >>> link or sink) so they will exist for all devices of that type >>> while some are device specific. >>> >>> Maybe I am misunderstanding the question but are you suggesting >>> to register CoreSight devices to the AMBA bus instead of the >>> CoreSight core layer code? >> >> Yes exactly. >> >>> Will Deacon mentioned earlier that AMBA framework can be changed >>> to accomodate devices with a different class but I am more >>> concerned with losing some of the stuff that the core layer code >>> does (eg. coresight_register, coresight_enable, coresight_disable >>> in coresight.c) if we register CoreSight drivers to the AMBA bus >>> without letting the core layer know about the device connections. >> >> I may be missing something, but couldn't you keep all the >> register/enable/disable stuff but just register the device with the amba >> bus? Obviously some changes would need to be made. >> > > Ok, so are you referring to making CoreSight devices register > with AMBA bus instead of platform bus keeping everything else > intact? Yes exactly. However, please note I am not saying that we should do this, and I asking what direction does the community want us to take here? Platform bus or AMBA bus? >> Personally, I don't have strong feeling either way, but we have ETM/ETB >> drivers using AMBA today and so I am hoping we can come to agreement on >> this going forward. >> >> Russell, Will, what are your thoughts? >> >> Otherwise, looking at the code, I like what you have implemented. I >> still need to look closer, but I am struggling to figure out how a >> coresight device such as the cross-trigger-interface fits with this >> model. This model appears to be geared towards coresight devices used >> for traces purposes and are either source, links or sinks. The >> cross-trigger-interface is not a source or a sink. However, although you >> it could be considered as a link (routing events), it is not really, as >> it may not link to other coresight sinks/source. >> >> In my case, I have PMU-IRQ --> CTI --> GIC. So a non-coresight source >> and sink. In away the CTI looks more like a 2nd-level interrupt >> controller than anything else. Hence, another type of coresight device >> may be needed in addition to source, links and sinks. Or link is >> extended in some way to connect to non-coresight sources/sinks. >> >> Let me know if you have any thoughts. >> > > I had left the "None" type for miscellaneous stuff but I agree it > should be a separate type - maybe "debug". > > Having said that I like the CTI driver you have uploaded. Need to > look at it in more detail. Since CTI connections can vary between > chip to chip, we need a generic way to deal with it. Yes if you have any ideas let me know. As Will had mentioned it would be good to have a common function table all these devices could use too. I will take a closer look at what you have. Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Remove inline from clock framework function definitions to build the kernel with GCC 4.7
On 11/15/2012 11:07 AM, Igor Mazanov wrote: > Remove inline from clock framework function definitions to > build the kernel with GCC 4.7 Adding Mike to the party ... May be good to add some details about the exact problem seen. I am seeing the same problem today with GCC 4.7 and Tony's master branch. For a bit of background it seems that for 4.7 not having the body of the inlined function available in the header is causing this error. Another example here [1]. The actual compiler error seen for OMAP is ... In file included from arch/arm/mach-omap2/clockdomain.c:25:0: arch/arm/mach-omap2/clockdomain.c: In function ‘clkdm_clk_disable’: include/linux/clk-provider.h:338:12: error: inlining failed in call to always_inline ‘__clk_get_enable_count’: function body not available arch/arm/mach-omap2/clockdomain.c:1001:28: error: called from here make[1]: *** [arch/arm/mach-omap2/clockdomain.o] Error 1 make: *** [arch/arm/mach-omap2] Error 2 > Signed-off-by: Igor Mazanov > --- > include/linux/clk-provider.h |4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > index c127315..f9f5e9e 100644 > --- a/include/linux/clk-provider.h > +++ b/include/linux/clk-provider.h > @@ -335,8 +335,8 @@ const char *__clk_get_name(struct clk *clk); > struct clk_hw *__clk_get_hw(struct clk *clk); > u8 __clk_get_num_parents(struct clk *clk); > struct clk *__clk_get_parent(struct clk *clk); > -inline int __clk_get_enable_count(struct clk *clk); > -inline int __clk_get_prepare_count(struct clk *clk); > +int __clk_get_enable_count(struct clk *clk); > +int __clk_get_prepare_count(struct clk *clk); > unsigned long __clk_get_rate(struct clk *clk); > unsigned long __clk_get_flags(struct clk *clk); > int __clk_is_enabled(struct clk *clk); Do we also need to remove the inline from the functions declared in drivers/clk/clk.c too? Cheers Jon [1] https://bugs.launchpad.net/linaro-android/+bug/983496 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] ARM: ux500: Describe UART platform registering issues more accurately
On 11/16/2012 03:36 AM, Arnd Bergmann wrote: > On Thursday 15 November 2012, Lee Jones wrote: >> On Thu, 15 Nov 2012, Arnd Bergmann wrote: >> >>> On Thursday 15 November 2012, Lee Jones wrote: UARTs no longer require call-back information, since the reset call-back was removed in 43b5f0d69291374f602ad8e1817f329573a59010. The only AUXDATA dependencies remaining for UARTs are DMA settings. Signed-off-by: Lee Jones >>> >>> Acked-by: Arnd Bergmann >>> >>> What is the state of the DMA binding now? We originally wanted to merge >>> it for 3.7, but that didn't work out, so I hope we can get it done for >>> 3.8. >> >> It's dead. The conversation has been stale for weeks. >> >> Perhaps you'd like to go and poke it? :) >> > > Vinod, > > is there anything you are waiting for still? Should Jon resend > his latest patches to make sure we get them merged this time? > > I have multiple people that want to send me patches for 3.8 based > on that work, so we are running out of time now. Vinod responded today saying it will be in for v3.8 [1]. Jon [1] http://article.gmane.org/gmane.linux.ports.arm.omap/89502 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: dts: omap4-panda: Add LED support into the panda DTS files
On 04/17/2013 02:09 PM, Dan Murphy wrote: > The GPIO for LED D1 on the omap4-panda a1-a3 rev and the omap4-panda-es > are different. > > Abstract away the pinmux and the LED definitions for the two boards. Just a heads-up but you should base this upon Benoit's for_3.10 branch [1] as there is now a omap4-panda-common.dtsi where the led stuff currently resides. Cheers Jon [1] http://git.kernel.org/cgit/linux/kernel/git/bcousson/linux-omap-dt.git/log/?h=for_3.10/dts -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] dma: of: Fix of_node reference leak
On 04/19/2013 05:10 AM, Arnd Bergmann wrote: > On Friday 19 April 2013, Lars-Peter Clausen wrote: >> of_dma_request_slave_channel() currently does not drop the reference to the >> dma_spec of_node if no DMA controller matching the of_node could be found. >> This >> patch fixes it by always calling of_node_put(). >> >> Signed-off-by: Lars-Peter Clausen > > Acked-by: Arnd Bergmann Thanks! FWIW ... Reviewed-by: Jon Hunter Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] dma:of: Use a mutex to protect the of_dma_list
On 04/19/2013 04:42 AM, Lars-Peter Clausen wrote:
> Currently the OF DMA code uses a spin lock to protect the of_dma_list from
> concurrent access and a per controller reference count to protect the
> controller
> from being freed while a request operation is in progress. If
> of_dma_controller_free() is called for a controller who's reference count is
> not
> zero it will return -EBUSY and not remove the controller. This is fine up
> until
> here, but leaves the question what the caller of of_dma_controller_free() is
> supposed to do if the controller couldn't be freed. The only viable solution
> for the caller is to spin on of_dma_controller_free() until it returns
> success.
> E.g.
>
> do {
> ret = of_dma_controller_free(dev->of_node)
> } while (ret != -EBUSY);
>
> This is rather ugly and unnecessary and non of the current users of
> of_dma_controller_free() check it's return value anyway. Instead protect the
> list by a mutex. The mutex will be held as long as a request operation is in
> progress. So if of_dma_controller_free() is called while a request operation
> is
> in progress it will be put to sleep and only wake up once the request
> operation
> has finished.
>
> This means that it is no longer possible to register or unregister OF DMA
> controllers from a context where it's not possible to sleep. But I doubt that
> we'll ever need this.
Change also means that of_dma_request_slave_channel() cannot be called
from a context where it is not possible to sleep too, right? May be
worth mentioning this in the changelog as well.
> Also rename of_dma_get_controller back to of_dma_find_controller.
>
> Signed-off-by: Lars-Peter Clausen
> ---
> drivers/dma/of-dma.c | 76
> +-
> include/linux/of_dma.h | 6 ++--
> 2 files changed, 22 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c
> index 2882403..7aa0864 100644
> --- a/drivers/dma/of-dma.c
> +++ b/drivers/dma/of-dma.c
> @@ -13,38 +13,31 @@
> #include
> #include
> #include
> -#include
> +#include
> #include
> #include
> #include
>
> static LIST_HEAD(of_dma_list);
> -static DEFINE_SPINLOCK(of_dma_lock);
> +static DEFINE_MUTEX(of_dma_lock);
>
> /**
> - * of_dma_get_controller - Get a DMA controller in DT DMA helpers list
> + * of_dma_find_controller - Get a DMA controller in DT DMA helpers list
> * @dma_spec:pointer to DMA specifier as found in the device tree
> *
> * Finds a DMA controller with matching device node and number for dma cells
> - * in a list of registered DMA controllers. If a match is found the use_count
> - * variable is increased and a valid pointer to the DMA data stored is
> retuned.
> - * A NULL pointer is returned if no match is found.
> + * in a list of registered DMA controllers. If a match is found a valid
> pointer
> + * to the DMA data stored is retuned. A NULL pointer is returned if no match
> is
> + * found.
> */
> -static struct of_dma *of_dma_get_controller(struct of_phandle_args *dma_spec)
> +static struct of_dma *of_dma_find_controller(struct of_phandle_args
> *dma_spec)
> {
> struct of_dma *ofdma;
>
> - spin_lock(&of_dma_lock);
> -
> list_for_each_entry(ofdma, &of_dma_list, of_dma_controllers)
> if ((ofdma->of_node == dma_spec->np) &&
> - (ofdma->of_dma_nbcells == dma_spec->args_count)) {
> - ofdma->use_count++;
> - spin_unlock(&of_dma_lock);
> + (ofdma->of_dma_nbcells == dma_spec->args_count))
> return ofdma;
> - }
> -
> - spin_unlock(&of_dma_lock);
>
> pr_debug("%s: can't find DMA controller %s\n", __func__,
>dma_spec->np->full_name);
> @@ -53,22 +46,6 @@ static struct of_dma *of_dma_get_controller(struct
> of_phandle_args *dma_spec)
> }
>
> /**
> - * of_dma_put_controller - Decrement use count for a registered DMA
> controller
> - * @of_dma: pointer to DMA controller data
> - *
> - * Decrements the use_count variable in the DMA data structure. This function
> - * should be called only when a valid pointer is returned from
> - * of_dma_get_controller() and no further accesses to data referenced by that
> - * pointer are needed.
> - */
> -static void of_dma_put_controller(struct of_dma *ofdma)
> -{
> - spin_lock(&of_dma_lock);
> - ofdma->use_count--;
> - spin_unlock(&of_dma_lock);
> -}
> -
> -/**
> * of_dma_controller_register - Register a DMA controller to DT DMA helpers
> * @np: device node of DMA controller
> * @of_dma_xlate:translation function which converts a phandle
> @@ -114,12 +91,11 @@ int of_dma_controller_register(struct device_node *np,
> ofdma->of_dma_nbcells = nbcells;
> ofdma->of_dma_xlate = of_dma_xlate;
> ofdma->of_dma_data = data;
> - ofdma->use_count = 0;
>
> /* Now queue of_dma controller
Re: [PATCH 2/2] dma:of: Use a mutex to protect the of_dma_list
On 04/20/2013 05:38 AM, Arnd Bergmann wrote: > On Saturday 20 April 2013, Lars-Peter Clausen wrote: >> On 04/20/2013 12:45 AM, Jon Hunter wrote: >>> I think that there is a problem here. For controllers using the >>> of_dma_simple_xlate(), this will call dma_request_channel() which also >>> uses a mutex. >> >> That would only be a problem if it'd use the same mutex. Holding two mutexes >> at >> the same time is not a problem per se. >> > > I guess Jon originlly tried it with a spinlock as the outer lock, which indeed > does not work, but the new version does not have this problem. Yes the spinlock was a problem and hence for the put/get functions. Ok so that's fine then. Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] dma:of: Use a mutex to protect the of_dma_list
On 04/19/2013 06:13 PM, Arnd Bergmann wrote: > On Saturday 20 April 2013, Jon Hunter wrote: >> Change also means that of_dma_request_slave_channel() cannot be called >> from a context where it is not possible to sleep too, right? May be >> worth mentioning this in the changelog as well. > > You already cannot do that, because it requires dma_list_mutex. Right in the case where you use of_dma_simple_xlate(). However, someone could implement whatever they wanted for the xlate. Probably not likely as it needs to return struct dma_chan, but could be possible. That's all. Not a big deal. Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] dma: of: Remove check on always true condition
On 04/22/2013 03:33 AM, Lars-Peter Clausen wrote:
> Both of_dma_nbcells field of the of_dma_controller and the args_count field of
> the dma_spec are initialized by parsing the #dma-cells attribute of their
> device
> tree node. So if the device tree nodes of a DMA controller and the dma_spec
> match this means that of_dma_nbcells and args_count will also match. So the
> second test in the of_dma_find_controller loop is redundant because given the
> first test yields true the second test will also yield true. So we can safely
> remove the test whether of_dma_nbcells matches args_count. Since this was the
> last user of the of_dma_nbcells field we can remove it altogether.
This assumes that someone has correctly added the dma information to the
dma slave binding. I could see systems where different dma controllers
have different of_dma_nbcells and so someone could put the enter wrong
number of cells for a dma slave binding. Its really to catch user error.
> Signed-off-by: Lars-Peter Clausen
> ---
> drivers/dma/of-dma.c | 14 +-
> include/linux/of_dma.h | 1 -
> 2 files changed, 1 insertion(+), 14 deletions(-)
>
> diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c
> index 268cc8a..75334bd 100644
> --- a/drivers/dma/of-dma.c
> +++ b/drivers/dma/of-dma.c
> @@ -35,8 +35,7 @@ static struct of_dma *of_dma_find_controller(struct
> of_phandle_args *dma_spec)
> struct of_dma *ofdma;
>
> list_for_each_entry(ofdma, &of_dma_list, of_dma_controllers)
> - if ((ofdma->of_node == dma_spec->np) &&
> - (ofdma->of_dma_nbcells == dma_spec->args_count))
> + if (ofdma->of_node == dma_spec->np)
> return ofdma;
Other device-tree functions perform similar tests to this such as ...
static int of_gpiochip_find_and_xlate(struct gpio_chip *gc, void *data)
{
struct gg_data *gg_data = data;
int ret;
if ((gc->of_node != gg_data->gpiospec.np) ||
(gc->of_gpio_n_cells != gg_data->gpiospec.args_count) ||
(!gc->of_xlate))
return false;
...
Cheers
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] dma: of: Remove check on always true condition
On 04/22/2013 04:00 PM, Lars-Peter Clausen wrote:
> On 04/22/2013 10:52 PM, Jon Hunter wrote:
>>
>> On 04/22/2013 03:33 AM, Lars-Peter Clausen wrote:
>>> Both of_dma_nbcells field of the of_dma_controller and the args_count field
>>> of
>>> the dma_spec are initialized by parsing the #dma-cells attribute of their
>>> device
>>> tree node. So if the device tree nodes of a DMA controller and the dma_spec
>>> match this means that of_dma_nbcells and args_count will also match. So the
>>> second test in the of_dma_find_controller loop is redundant because given
>>> the
>>> first test yields true the second test will also yield true. So we can
>>> safely
>>> remove the test whether of_dma_nbcells matches args_count. Since this was
>>> the
>>> last user of the of_dma_nbcells field we can remove it altogether.
>>
>> This assumes that someone has correctly added the dma information to the
>> dma slave binding. I could see systems where different dma controllers
>> have different of_dma_nbcells and so someone could put the enter wrong
>> number of cells for a dma slave binding. Its really to catch user error.
>
> No, this assumes nothing. The condition will _always_ be true.
>
> dma_spec->args_count is initialized by parsing the #dma-cells attribute of
> dma_sepc->np. of_dma->of_dma_nbcells is initialized by parsing the #dma-cells
> attribute of of_dma->of_node. If ofdma->of_node equals dma_spec->np then
> dma_spec->args_count will also equal of_dma->of_dma_nbcells.
Thanks for the clarification. I should have looked more closely at
of_parse_phandle_with_args().
Yes I agree it will always be true if count is less than or equal to
MAX_PHANDLE_ARGS (which defaults to 8). It is very unlikely that someone
would use more than 8 and I guess it does warn on this condition.
if (out_args) {
int i;
if (WARN_ON(count > MAX_PHANDLE_ARGS))
count = MAX_PHANDLE_ARGS;
out_args->np = node;
out_args->args_count = count;
for (i = 0; i < count; i++)
out_args->args[i] = be32_to_cpup(list++);
}
Cheers
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 11/14] Documentation: dt: binding: omap: am43x timer
On 28/05/13 23:05, Stephen Warren wrote: > On 05/28/2013 03:25 PM, Jon Hunter wrote: >> >> On 27/05/13 15:37, Afzal Mohammed wrote: >>> AM43x timer bindings. >>> >>> Signed-off-by: Afzal Mohammed >>> --- >>> Documentation/devicetree/bindings/arm/omap/timer.txt | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/arm/omap/timer.txt >>> b/Documentation/devicetree/bindings/arm/omap/timer.txt >>> index d02e27c..70cb398 100644 >>> --- a/Documentation/devicetree/bindings/arm/omap/timer.txt >>> +++ b/Documentation/devicetree/bindings/arm/omap/timer.txt >>> @@ -14,6 +14,8 @@ Required properties: >>> ti,omap5430-timer (applicable to OMAP543x devices) >>> ti,am335x-timer (applicable to AM335x devices) >>> ti,am335x-timer-1ms (applicable to AM335x devices) >>> + "ti,am4372-timer-1ms", "ti,am335x-timer-1ms" for AM43x >>> 1ms timer >>> + "ti,am4372-timer", "ti,am335x-timer" for AM43x timers >>> other than 1ms one >>> >>> - reg: Contains timer register address range (base >>> address and >>> length). >> >> If you are adding more compatibility strings, then this implies that the >> AM43x timers are not 100% compatible with any other device listed (such >> as am335x or any omap device). That's fine but you should state that in >> the changelog. If the AM43x timer registers are 100% compatible with >> existing devices you should not add these. > > I'm not sure that's true; .dts files should always include a compatible > value that describes the most specific model of the HW, plus any > baseline compatible value that the HW is compatible with. This allows > any required quirks/fixes/... to be applied for the specific HW model > later even if nobody knows right now they'll be needed. Hence, defining > new compatible values doesn't necessarily mean incompatible HW. Yes that does seem to agree with what Grant had told me in the past [1]. So this is ok then. However, I still think that the changelog needs to be vastly improved and more explicit about the changes. Cheers Jon [1] http://thread.gmane.org/gmane.linux.ports.arm.omap/90551/focus=26447 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mach_omap2: use PTR_RET instead of IS_ERR + PTR_ERR
On 03/22/2013 11:36 AM, Russell King - ARM Linux wrote:
> On Wed, Mar 20, 2013 at 01:28:47PM -0500, Jon Hunter wrote:
>> Sorry I am now not sure I follow you here. Someone just pointed out to
>> me that PTR_RET() is defined as ...
>>
>> static inline int __must_check PTR_RET(const void *ptr)
>> {
>> if (IS_ERR(ptr))
>> return PTR_ERR(ptr);
>> else
>> return 0;
>> }
>>
>> So the above change appears to be equivalent. Is there something that is
>> wrong with the current implementation that needs to be fixed?
>
> No - I misread it as PTR_ERR not PTR_RET. Your patch is fine.
Thanks for confirming. I had made the same mistake recently too!
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: OMAP2+: timer: initialize before using oh_name
On 28/05/13 07:24, Afzal Mohammed wrote:
> of_property_read_string_index(...,&oh_name) in omap_dm_timer_init_one
> does not alter the value of 'oh_name' even if the relevant function
> fails and as 'oh_name' in stack may have a non-zero value, it would
> be misunderstood by timer code that DT has specified "ti,hwmod"
> property for timer. 'oh_name' in this scenario would be a junk value,
> this would result in module not being enabled by hwmod API's for
> timer, and in turn crash.
>
> Signed-off-by: Afzal Mohammed
> ---
> arch/arm/mach-omap2/timer.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
> index f8b23b8..8e0c390 100644
> --- a/arch/arm/mach-omap2/timer.c
> +++ b/arch/arm/mach-omap2/timer.c
> @@ -220,7 +220,7 @@ static int __init omap_dm_timer_init_one(struct
> omap_dm_timer *timer,
>int posted)
> {
> char name[10]; /* 10 = sizeof("gptXX_Xck0") */
> - const char *oh_name;
> + const char *oh_name = NULL;
> struct device_node *np;
> struct omap_hwmod *oh;
> struct resource irq, mem;
Thanks!
Acked-by: Jon Hunter
Cheers
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 12/14] Documentation: dt: binding: omap: am43x counter
On 27/05/13 15:37, Afzal Mohammed wrote: > AM43x 32K counter binding. > > Signed-off-by: Afzal Mohammed > --- > Documentation/devicetree/bindings/arm/omap/counter.txt | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/devicetree/bindings/arm/omap/counter.txt > b/Documentation/devicetree/bindings/arm/omap/counter.txt > index 5bd8aa0..9c48dca 100644 > --- a/Documentation/devicetree/bindings/arm/omap/counter.txt > +++ b/Documentation/devicetree/bindings/arm/omap/counter.txt > @@ -2,6 +2,7 @@ OMAP Counter-32K bindings > > Required properties: > - compatible:Must be "ti,omap-counter32k" for OMAP controllers > + "ti,am4372-counter32k","ti,omap-counter32k" for AM43x counter > - reg: Contains timer register address range (base address and > length) > - ti,hwmods: Name of the hwmod associated to the counter, which is typically > "counter_32k" Changelog should state why this is needed. Jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 11/14] Documentation: dt: binding: omap: am43x timer
On 27/05/13 15:37, Afzal Mohammed wrote: > AM43x timer bindings. > > Signed-off-by: Afzal Mohammed > --- > Documentation/devicetree/bindings/arm/omap/timer.txt | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/arm/omap/timer.txt > b/Documentation/devicetree/bindings/arm/omap/timer.txt > index d02e27c..70cb398 100644 > --- a/Documentation/devicetree/bindings/arm/omap/timer.txt > +++ b/Documentation/devicetree/bindings/arm/omap/timer.txt > @@ -14,6 +14,8 @@ Required properties: > ti,omap5430-timer (applicable to OMAP543x devices) > ti,am335x-timer (applicable to AM335x devices) > ti,am335x-timer-1ms (applicable to AM335x devices) > + "ti,am4372-timer-1ms", "ti,am335x-timer-1ms" for AM43x > 1ms timer > + "ti,am4372-timer", "ti,am335x-timer" for AM43x timers > other than 1ms one > > - reg: Contains timer register address range (base > address and > length). If you are adding more compatibility strings, then this implies that the AM43x timers are not 100% compatible with any other device listed (such as am335x or any omap device). That's fine but you should state that in the changelog. If the AM43x timer registers are 100% compatible with existing devices you should not add these. Jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/3] mmc: omap_hsmmc: Skip platform_get_resource_byname() for dt case
On 03/06/2013 07:56 AM, Matt Porter wrote: > On Wed, Mar 06, 2013 at 07:12:29PM +0530, Balaji T K wrote: >> On Wednesday 06 March 2013 02:43 AM, Matt Porter wrote: >>> From: Santosh Shilimkar >>> >>> MMC driver probe will abort for DT case because of failed >>> platform_get_resource_byname() lookup. Fix it by skipping resource >>> byname lookup for device tree build. >>> >>> Issue is hidden because hwmod popullates the IO resources which >>> helps to succeed platform_get_resource_byname() and probe. >>> >> >> Hi Matt, >> Could you please drop this patch from the current series, >> since this patch causes regression on omap3,4 platform >> which are not yet dma dt adapted. >> It is best to send this patch along with Jon Hunter dma dt series, >> which adds dt dma support and mmc dma data. DMA dt series is needed >> any way before hwmod cleanup can happen. > > *sigh* ok, I should have never split this stuff out from the am33xx > series. Will do. There will not be any regression if all of these make the same merge window. I am hoping that the omap dma patches will make 3.10 as well. I think that it is best for Matt to keep his patches together although now I see he has already posted a V3 dropping this :-( Jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/5] arm: dts: omap4-panda: Add I2c pinctrl data
On 02/13/2013 03:28 AM, Sourav Poddar wrote:
> Booting 3.8-rc6 on omap4 panda results in the following error
>
> [0.27] omap_i2c 4807.i2c: did not get pins for i2c error: -19
> [0.445770] omap_i2c 4807.i2c: bus 0 rev0.11 at 400 kHz
> [0.473937] omap_i2c 48072000.i2c: did not get pins for i2c error: -19
> [0.474670] omap_i2c 48072000.i2c: bus 1 rev0.11 at 400 kHz
> [0.474822] omap_i2c 4806.i2c: did not get pins for i2c error: -19
> [0.476379] omap_i2c 4806.i2c: bus 2 rev0.11 at 100 kHz
> [0.477294] omap_i2c 4835.i2c: did not get pins for i2c error: -19
> [0.477996] omap_i2c 4835.i2c: bus 3 rev0.11 at 400 kHz
> [0.483398] Switching to clocksource 32k_counter
>
> This happens because omap4 panda dts file is not adapted to use i2c through
> pinctrl framework. Populating i2c pinctrl data to get rid of the error.
What about the panda-es and panda-a4?
> Tested on omap4460 panda with 3.8-rc6 kernel.
>
> Signed-off-by: Sourav Poddar
> Reported-by: Luciano Coelho
> ---
> arch/arm/boot/dts/omap4-panda.dts | 40
> +
> 1 files changed, 40 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/boot/dts/omap4-panda.dts
> b/arch/arm/boot/dts/omap4-panda.dts
> index 4122efe..f951e6b 100644
> --- a/arch/arm/boot/dts/omap4-panda.dts
> +++ b/arch/arm/boot/dts/omap4-panda.dts
> @@ -110,9 +110,40 @@
> 0x58 0x10b /* hdmi_hpd.gpio_63 INPUT PULLDOWN |
> MODE3 */
> >;
> };
> +
> + i2c1_pins: pinmux_i2c1_pins {
> + pinctrl-single,pins = <
> + 0xe2 0x118/* i2c1_scl PULLUP | INPUTENABLE |
> MODE0 */
> + 0xe4 0x118/* i2c1_sda PULLUP | INPUTENABLE |
> MODE0 */
> + >;
> + };
> +
> + i2c2_pins: pinmux_i2c2_pins {
> + pinctrl-single,pins = <
> + 0xe6 0x118/* i2c2_scl PULLUP | INPUTENABLE |
> MODE0 */
> + 0xe8 0x118/* i2c2_sda PULLUP | INPUTENABLE |
> MODE0 */
> + >;
> + };
> +
> + i2c3_pins: pinmux_i2c3_pins {
> + pinctrl-single,pins = <
> + 0xea 0x118/* i2c3_scl PULLUP | INPUTENABLE |
> MODE0 */
> + 0xec 0x118 /* i2c3_sda PULLUP | INPUTENABLE | MODE0
> */
> + >;
> + };
> +
> + i2c4_pins: pinmux_i2c4_pins {
> + pinctrl-single,pins = <
> + 0xee 0x118/* i2c4_scl PULLUP | INPUTENABLE |
> MODE0 */
> + 0xf0 0x118 /* i2c4_sda PULLUP | INPUTENABLE | MODE0
> */
> + >;
> + };
> };
A quick look at the data manual shows that omap4430 and omap4460 has the
same pin mux options for i2c. Furthermore, the data manual shows only
one mux option for i2c1-4. Therefore, should these mux options be placed
in omap4.dtsi? Boards not using specific i2c controllers can disabled
them in there board dts file (same way we do for mmc).
Cheers
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/5] arm: dts: omap4-panda: Add I2c pinctrl data
On 02/13/2013 09:57 AM, Jon Hunter wrote:
>
> On 02/13/2013 03:28 AM, Sourav Poddar wrote:
>> Booting 3.8-rc6 on omap4 panda results in the following error
>>
>> [0.27] omap_i2c 4807.i2c: did not get pins for i2c error: -19
>> [0.445770] omap_i2c 4807.i2c: bus 0 rev0.11 at 400 kHz
>> [0.473937] omap_i2c 48072000.i2c: did not get pins for i2c error: -19
>> [0.474670] omap_i2c 48072000.i2c: bus 1 rev0.11 at 400 kHz
>> [0.474822] omap_i2c 4806.i2c: did not get pins for i2c error: -19
>> [0.476379] omap_i2c 4806.i2c: bus 2 rev0.11 at 100 kHz
>> [0.477294] omap_i2c 4835.i2c: did not get pins for i2c error: -19
>> [0.477996] omap_i2c 4835.i2c: bus 3 rev0.11 at 400 kHz
>> [0.483398] Switching to clocksource 32k_counter
>>
>> This happens because omap4 panda dts file is not adapted to use i2c through
>> pinctrl framework. Populating i2c pinctrl data to get rid of the error.
>
> What about the panda-es and panda-a4?
>
>> Tested on omap4460 panda with 3.8-rc6 kernel.
>>
>> Signed-off-by: Sourav Poddar
>> Reported-by: Luciano Coelho
>> ---
>> arch/arm/boot/dts/omap4-panda.dts | 40
>> +
>> 1 files changed, 40 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/omap4-panda.dts
>> b/arch/arm/boot/dts/omap4-panda.dts
>> index 4122efe..f951e6b 100644
>> --- a/arch/arm/boot/dts/omap4-panda.dts
>> +++ b/arch/arm/boot/dts/omap4-panda.dts
>> @@ -110,9 +110,40 @@
>> 0x58 0x10b /* hdmi_hpd.gpio_63 INPUT PULLDOWN |
>> MODE3 */
>> >;
>> };
>> +
>> +i2c1_pins: pinmux_i2c1_pins {
>> +pinctrl-single,pins = <
>> +0xe2 0x118/* i2c1_scl PULLUP | INPUTENABLE |
>> MODE0 */
>> +0xe4 0x118/* i2c1_sda PULLUP | INPUTENABLE |
>> MODE0 */
>> +>;
>> +};
>> +
>> +i2c2_pins: pinmux_i2c2_pins {
>> +pinctrl-single,pins = <
>> +0xe6 0x118/* i2c2_scl PULLUP | INPUTENABLE |
>> MODE0 */
>> +0xe8 0x118/* i2c2_sda PULLUP | INPUTENABLE |
>> MODE0 */
>> +>;
>> +};
>> +
>> +i2c3_pins: pinmux_i2c3_pins {
>> +pinctrl-single,pins = <
>> +0xea 0x118/* i2c3_scl PULLUP | INPUTENABLE |
>> MODE0 */
>> +0xec 0x118 /* i2c3_sda PULLUP | INPUTENABLE | MODE0
>> */
>> +>;
>> +};
>> +
>> +i2c4_pins: pinmux_i2c4_pins {
>> +pinctrl-single,pins = <
>> +0xee 0x118/* i2c4_scl PULLUP | INPUTENABLE |
>> MODE0 */
>> +0xf0 0x118 /* i2c4_sda PULLUP | INPUTENABLE | MODE0
>> */
>> +>;
>> +};
>> };
>
> A quick look at the data manual shows that omap4430 and omap4460 has the
> same pin mux options for i2c. Furthermore, the data manual shows only
> one mux option for i2c1-4. Therefore, should these mux options be placed
> in omap4.dtsi? Boards not using specific i2c controllers can disabled
> them in there board dts file (same way we do for mmc).
I guess for i2c, a given omap4 board may use external pull-ups and not
use the internal ones and so putting this in the omap4.dtsi may not be
desirable. However, it seems that a common omap4-panda.dtsi could be
used here.
Cheers
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] ARM: OMAP4: Add OMAP4 Blaze Tablet support
On 02/13/2013 05:28 PM, Ruslan Bilovol wrote: > Hi Tony, Jon, > > On Mon, Feb 11, 2013 at 9:00 PM, Tony Lindgren wrote: >> * Jon Hunter [130211 10:58]: >>> >>> Please note that the blaze is derived from the omap4-sdp board and so I >>> would hope that we can use the existing for sdp dts and board file for >>> blaze. In fact this is what I do today for basic booting. >>> >>> So unless there is some feature on the blaze that is not compatible with >>> the original sdp, we should just use the sdp dts. >> >> Sounds like we need some common .dts file and separate files >> for sdp, blaze and tablet that include the common .dts file? >> >> There's a different LCD panel at least between blaze and the >> tablet. > > Please note, that, although 'Blaze' board is very close to 'SDP' board, > there are quite big differences comparing to 'Blaze Tablet' board. > At least: > - LCD panels > - touchscreen controllers > - LEDs > - Keypad (on 'Blaze') / gpio keys (on 'Blaze Tablet') > - Sensors > - HS USB Host related stuff SDP also has usb host too (but yes blaze does not). > - cameras (there is no embedded cameras on 'Blaze Tablet' board) > > As per my point of view, next should be done: > 1) Add the DTS file for BlazeTablet board > 2) Figure out what is common for both and move it to some common file > (if that makes sense) > > I'm currently working on step '#1'. So after that step #2 should not > be an issue for us. Sounds good. They all use the same processor boards and so may be that can be common in DT and we can have a dtsi for that. Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] OMAP: add pwm driver using dmtimers.
Hi Neil,
On 12/12/2012 02:24 AM, NeilBrown wrote:
>
>
> This patch is based on an earlier patch by Grant Erickson
> which provided pwm devices using the 'legacy' interface.
>
> This driver instead uses the new framework interface.
>
> Cc: Grant Erickson
> Signed-off-by: NeilBrown
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index ed81720..7df573a 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -85,6 +85,15 @@ config PWM_MXS
> To compile this driver as a module, choose M here: the module
> will be called pwm-mxs.
>
> +config PWM_OMAP
> + tristate "OMAP pwm support"
> + depends on ARCH_OMAP
We should probably have depends on or selects OMAP_DM_TIMER here too.
> + help
> + Generic PWM framework driver for OMAP
> +
> + To compile this driver as a module, choose M here: the module
> + will be called pwm-omap
> +
> config PWM_PUV3
> tristate "PKUnity NetBook-0916 PWM support"
> depends on ARCH_PUV3
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index acfe482..f5d200d 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -5,6 +5,7 @@ obj-$(CONFIG_PWM_IMX) += pwm-imx.o
> obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
> obj-$(CONFIG_PWM_LPC32XX)+= pwm-lpc32xx.o
> obj-$(CONFIG_PWM_MXS)+= pwm-mxs.o
> +obj-$(CONFIG_PWM_OMAP) += pwm-omap.o
> obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o
> obj-$(CONFIG_PWM_PXA)+= pwm-pxa.o
> obj-$(CONFIG_PWM_SAMSUNG)+= pwm-samsung.o
> diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
> new file mode 100644
> index 000..e3dbce3
> --- /dev/null
> +++ b/drivers/pwm/pwm-omap.c
> @@ -0,0 +1,318 @@
> +/*
> + *Copyright (c) 2012 NeilBrown
> + *Heavily based on earlier code which is:
> + *Copyright (c) 2010 Grant Erickson
> + *
> + *Also based on pwm-samsung.c
> + *
> + *This program is free software; you can redistribute it and/or
> + *modify it under the terms of the GNU General Public License
> + *version 2 as published by the Free Software Foundation.
> + *
> + *Description:
> + * This file is the core OMAP2/3 support for the generic, Linux
I would drop the OMAP2/3 and just say OMAP here. Potentially this should
work for OMAP1-5.
> + * PWM driver / controller, using the OMAP's dual-mode timers.
> + *
> + *The 'id' number for the device encodes the number of the dm timer
> + *to use, and the polarity of the output.
> + *lsb is '1' of active-high, and '0' for active low
> + *remaining bit a timer number and need to be shifted down before use.
> + */
> +
> +#define pr_fmt(fmt) "pwm-omap: " fmt
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
This is going to be a problem for the single zImage work, because we
cannot include any plat headers in driver code any more. Therefore,
although it is not ideal, one way to handle this is pass function
pointers to the various dmtimer APIs that are needed via the platform
data. Painful I know ...
> +#define DM_TIMER_LOAD_MIN0xFFFE
> +
> +struct omap_chip {
> + struct platform_device *pdev;
> +
> + struct omap_dm_timer*dm_timer;
> + unsigned intpolarity;
> + const char *label;
> +
> + unsigned intduty_ns, period_ns;
> + struct pwm_chip chip;
> +};
> +
> +#define to_omap_chip(chip) container_of(chip, struct omap_chip, chip)
> +
> +#define pwm_dbg(_pwm, msg...) dev_dbg(&(_pwm)->pdev->dev, msg)
> +
> +/**
> + * pwm_calc_value - determines the counter value for a clock rate and period.
> + * @clk_rate: The clock rate, in Hz, of the PWM's clock source to compute the
> + *counter value for.
> + * @ns: The period, in nanoseconds, to computer the counter value for.
> + *
> + * Returns the PWM counter value for the specified clock rate and period.
> + */
> +static inline int pwm_calc_value(unsigned long clk_rate, int ns)
> +{
> + const unsigned long nanoseconds_per_second = 10;
> + int cycles;
> + __u64 c;
> +
> + c = (__u64)clk_rate * ns;
> + do_div(c, nanoseconds_per_second);
> + cycles = c;
> +
> + return DM_TIMER_LOAD_MIN - cycles;
> +}
> +
> +static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct omap_chip *omap = to_omap_chip(chip);
> + int status = 0;
> +
> + /* Enable the counter--always--before attempting to write its
> + * registers and then set the timer to its minimum load value to
> + * ensure we get an overflow event right away once we start it.
> + */
> +
> + omap_dm_timer_enable(omap->dm_timer);
> + omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN);
> + omap_dm_timer_start(omap->dm_timer);
> + omap_dm_timer_disable(omap->dm_timer);
Why not just use
Re: [PATCH] OMAP: add pwm driver using dmtimers.
On 12/12/2012 05:31 AM, Thierry Reding wrote:
> On Wed, Dec 12, 2012 at 07:24:30PM +1100, NeilBrown wrote:
[snip]
>> +static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> +struct omap_chip *omap = to_omap_chip(chip);
>> +int status = 0;
>> +
>> +/* Enable the counter--always--before attempting to write its
>> + * registers and then set the timer to its minimum load value to
>> + * ensure we get an overflow event right away once we start it.
>> + */
>
> Block comments should be in the following format:
>
> /*
>* foo...
>* bar...
>*/
>
>> +
>> +omap_dm_timer_enable(omap->dm_timer);
>> +omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN);
>> +omap_dm_timer_start(omap->dm_timer);
>> +omap_dm_timer_disable(omap->dm_timer);
>
> So omap_dm_timer_disable() doesn't actually stop the timer? It just
> disables the access to the registers?
I thought this looked odd too ;-)
So what is going on here is that omap_dm_timer_start() calls
omap_dm_timer_enable() but does not call omap_dm_timer_disable(). So the
last disable really just complements the first enable (ie. decrements
the use count), but the timer will not actually be disabled, because the
start has called an extra enable.
These four function calls can be replaced by one call to
omap_dm_timer_set_load_start() and I think that will be much clearer and
concise.
In general, it should not be necessary to call these
omap_dm_timer_enable/disable APIs directly. I am not sure what the
history is or if there is a use-case that really requires this. So in
the future may be I should make them static so they cannot be used
directly :-)
Cheers
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] OMAP: add pwm driver using dmtimers.
On 12/12/2012 09:06 PM, NeilBrown wrote:
>
> [Thierry: question for you near the end - thanks]
>
> On Wed, 12 Dec 2012 10:08:28 -0600 Jon Hunter wrote:
>
>> Hi Neil,
>>
>> On 12/12/2012 02:24 AM, NeilBrown wrote:
>>>
>>>
>>> This patch is based on an earlier patch by Grant Erickson
>>> which provided pwm devices using the 'legacy' interface.
>>>
>>> This driver instead uses the new framework interface.
>>>
>>> Cc: Grant Erickson
>>> Signed-off-by: NeilBrown
>>>
>>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>>> index ed81720..7df573a 100644
>>> --- a/drivers/pwm/Kconfig
>>> +++ b/drivers/pwm/Kconfig
>>> @@ -85,6 +85,15 @@ config PWM_MXS
>>> To compile this driver as a module, choose M here: the module
>>> will be called pwm-mxs.
>>>
>>> +config PWM_OMAP
>>> + tristate "OMAP pwm support"
>>> + depends on ARCH_OMAP
>>
>> We should probably have depends on or selects OMAP_DM_TIMER here too.
>
> Sounds sensible - fixed.
>
>>
>>> + help
>>> + Generic PWM framework driver for OMAP
>>> +
>>> + To compile this driver as a module, choose M here: the module
>>> + will be called pwm-omap
>>> +
>>> config PWM_PUV3
>>> tristate "PKUnity NetBook-0916 PWM support"
>>> depends on ARCH_PUV3
>>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>>> index acfe482..f5d200d 100644
>>> --- a/drivers/pwm/Makefile
>>> +++ b/drivers/pwm/Makefile
>>> @@ -5,6 +5,7 @@ obj-$(CONFIG_PWM_IMX) += pwm-imx.o
>>> obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
>>> obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o
>>> obj-$(CONFIG_PWM_MXS) += pwm-mxs.o
>>> +obj-$(CONFIG_PWM_OMAP) += pwm-omap.o
>>> obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o
>>> obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
>>> obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
>>> diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
>>> new file mode 100644
>>> index 000..e3dbce3
>>> --- /dev/null
>>> +++ b/drivers/pwm/pwm-omap.c
>>> @@ -0,0 +1,318 @@
>>> +/*
>>> + *Copyright (c) 2012 NeilBrown
>>> + *Heavily based on earlier code which is:
>>> + *Copyright (c) 2010 Grant Erickson
>>> + *
>>> + *Also based on pwm-samsung.c
>>> + *
>>> + *This program is free software; you can redistribute it and/or
>>> + *modify it under the terms of the GNU General Public License
>>> + *version 2 as published by the Free Software Foundation.
>>> + *
>>> + *Description:
>>> + * This file is the core OMAP2/3 support for the generic, Linux
>>
>> I would drop the OMAP2/3 and just say OMAP here. Potentially this should
>> work for OMAP1-5.
>>
>
> Done.
>
>
>>> + * PWM driver / controller, using the OMAP's dual-mode timers.
>>> + *
>>> + *The 'id' number for the device encodes the number of the dm timer
>>> + *to use, and the polarity of the output.
>>> + *lsb is '1' of active-high, and '0' for active low
>>> + *remaining bit a timer number and need to be shifted down before use.
>>> + */
>>> +
>>> +#define pr_fmt(fmt) "pwm-omap: " fmt
>>> +
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +
>>> +#include
>>
>> This is going to be a problem for the single zImage work, because we
>> cannot include any plat headers in driver code any more. Therefore,
>> although it is not ideal, one way to handle this is pass function
>> pointers to the various dmtimer APIs that are needed via the platform
>> data. Painful I know ...
>
> But that doesn't work with devicetree does it?
Ugh, you are right! This is becoming an increasing problem.
> Can't we move the dmtimer.h file to include/linux/omap-dmtimer.h or something?
I can ask Tony if he thinks we could do that.
> It only included other things from include/linux, so it should be safe.
>
>>
>>> +#define DM_TIMER_LOAD_MIN 0xFFFE
>>> +
>>> +struct omap_chip {
>>> + struct platform_
Re: [PATCH] OMAP: add pwm driver using dmtimers.
On 12/12/2012 10:33 PM, NeilBrown wrote: > On Thu, 13 Dec 2012 14:06:35 +1100 NeilBrown wrote: > + omap_dm_timer_enable(omap->dm_timer); >>> >>> Do you need to call omap_dm_timer_enable here? _set_load and _set_match >>> will enable the timer. So this should not be necessary. >> >> True. That is what you get for copying someone else's code and not >> understanding it fully. > > However omap_dm_timer_write_counter *doesn't* enable the timer, and > explicitly checks that it is already runtime-enabled. > > Does that mean I don't need to call omap_dm_timer_write_counter here? Or > does it mean that I do need the enable/disable pair? Typically, omap_dm_timer_write_counter() is used to update the counter value while the counter is running and hence is enabled. Looking at the code, some more I now see what they are trying to do. It seems that they are trying to force an overflow to occur as soon as they enable the timer. This will cause the timer to load the count value from the timer load register into the timer counter register. So that does make sense to me. However, this should not be necessary as omap_dm_timer_set_load should do this for you. Therefore, I think that you could accomplish the same thing by doing ... omap_pwm_config --> omap_dm_timer_set_load() --> omap_dm_timer_set_match() --> omap_dm_timer_set_pwm() omap_pwm_enable --> omap_dm_timer_start() If we call _set_load in config then we don't need to call _load_start in the enable, we can just call _start. Can you try this and see if this is working ok? Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] OMAP: add pwm driver using dmtimers.
On 01/06/2013 03:12 PM, NeilBrown wrote:
[snip]
> I've been playing with off-mode and discovered that the first attempt to set
> the PWM after resume didn't work, but subsequent ones did.
> I did some digging and came up with the following patch.
> With this in place, the omap_pwm_suspend() above is definitely pointless (was
> wasn't really useful even without it).
Thanks for sending. I have given this patch a try on omap3 and I am still
some some failures with my timer read test. I need to dig into that further,
but I am guessing not related to your patch as there were problems there
before :-(
Some minor comments below ...
> NeilBrown
>
>
> From: NeilBrown
> Date: Mon, 7 Jan 2013 07:53:03 +1100
> Subject: [PATCH] OMAP dmtimer - simplify context-loss handling.
Nit, subject should formatted "ARM: OMAP: blah blah blah"
Also, may be worth calling this "fix context-loss" as this is really
fixing something that is broken.
> The context loss handling in dmtimer appears to assume that
>omap_dm_timer_set_load_start() or omap_dm_timer_start()
> and
>omap_dm_timer_stop()
>
> bracket all interactions. Only the first two restore the context and
> the last updates the context loss counter.
> However omap_dm_timer_set_load() or omap_dm_timer_set_match() can
> reasonably be called outside this bracketing, and the fact that they
> call omap_dm_timer_enable() / omap_dm_timer_disable() suggest that
> is expected.
>
> So if, after a transition into and out of off-mode which would cause
> the dm timer to loose all state, omap_dm_timer_set_match() is called
> before omap_dm_timer_start(), the value read from OMAP_TIMER_CTRL_REG
> will be 'wrong' and this wrong value will be stored context.tclr so
> a subsequent omap_dm_timer_start() can fail (As the control register
> is wrong).
>
> Simplify this be doing the restore-from-context in
> omap_dm_timer_enable() so that whenever the timer is enabled, the
> context is correct.
> Also update the ctx_loss_count at the same time as we notice it is
> wrong - these is no value in delaying this until the
> omap_dm_timer_disable() as it cannot change while the timer is
> enabled.
>
> Signed-off-by: NeilBrown
>
> diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
> index 938b50a..c216696 100644
> --- a/arch/arm/plat-omap/dmtimer.c
> +++ b/arch/arm/plat-omap/dmtimer.c
> @@ -253,6 +253,15 @@ EXPORT_SYMBOL_GPL(omap_dm_timer_free);
> void omap_dm_timer_enable(struct omap_dm_timer *timer)
> {
> pm_runtime_get_sync(&timer->pdev->dev);
> +
> + if (!(timer->capability & OMAP_TIMER_ALWON)) {
> + int loss_count =
> + omap_pm_get_dev_context_loss_count(&timer->pdev->dev);
> + if (loss_count != timer->ctx_loss_count) {
> + omap_timer_restore_context(timer);
> + timer->ctx_loss_count = loss_count;
> + }
> + }
> }
Can you rebase on v3.8-rc2? We no longer call
omap_pm_get_dev_context_loss_count() directly and so this
does not apply. Should be something like ...
diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index d51b75b..2c48182 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -315,7 +315,19 @@ EXPORT_SYMBOL_GPL(omap_dm_timer_free);
void omap_dm_timer_enable(struct omap_dm_timer *timer)
{
+ int c;
+
pm_runtime_get_sync(&timer->pdev->dev);
+
+ if (!(timer->capability & OMAP_TIMER_ALWON)) {
+ if (timer->get_context_loss_count) {
+ c = timer->get_context_loss_count(&timer->pdev->dev);
+ if (c != timer->ctx_loss_count) {
+ omap_timer_restore_context(timer);
+ timer->ctx_loss_count = c;
+ }
+ }
+ }
Cheers
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] omap: DT node Timer iteration fix
Hi Pantelis,
On 01/08/2013 07:31 AM, Pantelis Antoniou wrote:
> The iterator correctly handles of_node_put() calls.
> Remove it before continue'ing the loop.
> Without this patch you get:
Thanks for the fix!
May be worth mentioning that this will only be seen with
"CONFIG_OF_DYNAMIC" (and explains why I did not catch this one!).
> ERROR: Bad of_node_put() on /ocp/timer@44e31000!
> [] (unwind_backtrace+0x0/0xe0) from []
> (of_node_release+0x2c/0xa0)!
> [] (of_node_release+0x2c/0xa0) from []
> (of_find_matching_node_and_match+0x78/0x90)!
> [] (of_find_matching_node_and_match+0x78/0x90) from []
> (omap_get_timer_dt+0x78/0x90)!
> [] (omap_get_timer_dt+0x78/0x90) from []
> (omap_dm_timer_init_one.clone.2+0x34/0x2bc)!
> [] (omap_dm_timer_init_one.clone.2+0x34/0x2bc) from []
> (omap2_gptimer_clocksource_init.clone.4+0x24/0xa8)!
> [] (omap2_gptimer_clocksource_init.clone.4+0x24/0xa8) from
> [] (time_init+0x20/0x30)!
> [] (time_init+0x20/0x30) from []
> (start_kernel+0x1a8/0x2fc)!
>
> Signed-off-by: Pantelis Antoniou
> ---
> arch/arm/mach-omap2/timer.c | 8 ++--
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
> index 691aa67..b8ad6e6 100644
> --- a/arch/arm/mach-omap2/timer.c
> +++ b/arch/arm/mach-omap2/timer.c
> @@ -165,15 +165,11 @@ static struct device_node * __init
> omap_get_timer_dt(struct of_device_id *match,
> struct device_node *np;
>
> for_each_matching_node(np, match) {
> - if (!of_device_is_available(np)) {
> - of_node_put(np);
> + if (!of_device_is_available(np))
> continue;
> - }
>
> - if (property && !of_get_property(np, property, NULL)) {
> - of_node_put(np);
> + if (property && !of_get_property(np, property, NULL))
> continue;
> - }
>
> of_add_property(np, &device_disabled);
> return np;
Otherwise ...
Acked-by: Jon Hunter
Cheers
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RFC PATCH 3/3] dt-bindings: Add support for devices with multiple PM domains
Now that the generic PM domain framework supports devices that require multiple PM domains, update the device-tree binding for generic PM domains to state that one or more PM domain is permitted for a device. Signed-off-by: Jon Hunter --- Documentation/devicetree/bindings/power/power_domain.txt | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/power/power_domain.txt b/Documentation/devicetree/bindings/power/power_domain.txt index 025b5e7df61c..12131159b605 100644 --- a/Documentation/devicetree/bindings/power/power_domain.txt +++ b/Documentation/devicetree/bindings/power/power_domain.txt @@ -20,8 +20,9 @@ Required properties: as specified by device tree binding documentation of particular provider. Optional properties: - - power-domains : A phandle and PM domain specifier as defined by bindings of - the power controller specified by phandle. + - power-domains : An array of one or more PM domain specifiers (defined by the + bindings of the PM domain provider) for each PM domain that + is required by the device. Some power domains might be powered from another power domain (or have other hardware specific dependencies). For representing such dependency a standard PM domain consumer binding is used. When provided, all domains -- 2.1.4
[RFC PATCH 0/3] PM / Domains: Add support for devices that require multiple domains
The Tegra124/210 XUSB subsystem (that consists of both host and device
controllers) is partitioned across 3 PM domains which are:
- XUSBA: Superspeed logic (for USB 3.0)
- XUSBB: Device controller
- XUSBC: Host controller
These power domains are not nested and can be powered-up and down
independently of one another. In practice different scenarios require
different combinations of the power domains, for example:
- Superspeed host: XUSBA and XUSBC
- Superspeed device: XUSBA and XUSBB
Although it could be possible to logically nest both the XUSBB and XUSBC
domains under the XUSBA, superspeed may not always be used/required and
so this would keep it on unnecessarily.
Given that Tegra uses device-tree for describing the hardware, it would
be ideal that the device-tree 'power-domains' property for generic PM
domains could be extended to allow more than one PM domain to be
specified. For example, define the following the Tegra210 xHCI device ...
usb@7009 {
compatible = "nvidia,tegra210-xusb";
...
power-domains = <&pd_xusbhost>, <&pd_xusbss>;
};
This RFC extends the generic PM domain framework to allow a device to
define more than one PM domain in the device-tree 'power-domains'
property.
Jon Hunter (3):
PM / Domains: Add helper functions for finding and attaching PM
domains
PM / Domains: Add support for devices with multiple domains
dt-bindings: Add support for devices with multiple PM domains
.../devicetree/bindings/power/power_domain.txt | 5 +-
drivers/base/power/domain.c| 205 +++--
2 files changed, 155 insertions(+), 55 deletions(-)
--
2.1.4
[RFC PATCH 1/3] PM / Domains: Add helper functions for finding and attaching PM domains
In preparation for supporting devices that require more than one PM
domain, move the code for finding and attaching PM domains into local
helper functions.
Signed-off-by: Jon Hunter
---
drivers/base/power/domain.c | 121 ++--
1 file changed, 73 insertions(+), 48 deletions(-)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index b0cf46dcae73..382735949591 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1779,6 +1779,41 @@ struct generic_pm_domain *of_genpd_remove_last(struct
device_node *np)
}
EXPORT_SYMBOL_GPL(of_genpd_remove_last);
+static struct generic_pm_domain *genpd_dev_pm_lookup(struct device *dev,
+unsigned int index)
+{
+ struct of_phandle_args pd_args;
+ struct generic_pm_domain *pd;
+ int ret;
+
+ ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
+"#power-domain-cells", index,
&pd_args);
+ if (ret < 0) {
+ if (ret != -ENOENT)
+ return ERR_PTR(ret);
+
+ /*
+* Try legacy Samsung-specific bindings
+* (for backwards compatibility of DT ABI)
+*/
+ pd_args.args_count = 0;
+ pd_args.np = of_parse_phandle(dev->of_node,
+ "samsung,power-domain", 0);
+ if (!pd_args.np)
+ return ERR_PTR(-ENOENT);
+ }
+
+ pd = genpd_get_from_provider(&pd_args);
+ of_node_put(pd_args.np);
+ if (IS_ERR(pd)) {
+ dev_dbg(dev, "%s() failed to find PM domain: %ld\n",
+ __func__, PTR_ERR(pd));
+ return ERR_PTR(-EPROBE_DEFER);
+ }
+
+ return pd;
+}
+
/**
* genpd_dev_pm_detach - Detach a device from its PM domain.
* @dev: Device to detach.
@@ -1829,6 +1864,40 @@ static void genpd_dev_pm_sync(struct device *dev)
genpd_queue_power_off_work(pd);
}
+static int genpd_dev_pm_attach_device(struct device *dev,
+ struct generic_pm_domain *pd)
+{
+ unsigned int i;
+ int ret;
+
+ dev_dbg(dev, "adding to PM domain %s\n", pd->name);
+
+ for (i = 1; i < GENPD_RETRY_MAX_MS; i <<= 1) {
+ ret = genpd_add_device(pd, dev, NULL);
+ if (ret != -EAGAIN)
+ break;
+
+ mdelay(i);
+ cond_resched();
+ }
+
+ if (ret < 0) {
+ dev_err(dev, "failed to add to PM domain %s: %d",
+ pd->name, ret);
+ goto out;
+ }
+
+ dev->pm_domain->detach = genpd_dev_pm_detach;
+ dev->pm_domain->sync = genpd_dev_pm_sync;
+
+ mutex_lock(&pd->lock);
+ ret = genpd_poweron(pd, 0);
+ mutex_unlock(&pd->lock);
+out:
+ return ret ? -EPROBE_DEFER : 0;
+
+}
+
/**
* genpd_dev_pm_attach - Attach a device to its PM domain using DT.
* @dev: Device to attach.
@@ -1846,9 +1915,7 @@ static void genpd_dev_pm_sync(struct device *dev)
*/
int genpd_dev_pm_attach(struct device *dev)
{
- struct of_phandle_args pd_args;
struct generic_pm_domain *pd;
- unsigned int i;
int ret;
if (!dev->of_node)
@@ -1857,59 +1924,17 @@ int genpd_dev_pm_attach(struct device *dev)
if (dev->pm_domain)
return -EEXIST;
- ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
- "#power-domain-cells", 0, &pd_args);
- if (ret < 0) {
- if (ret != -ENOENT)
- return ret;
-
- /*
-* Try legacy Samsung-specific bindings
-* (for backwards compatibility of DT ABI)
-*/
- pd_args.args_count = 0;
- pd_args.np = of_parse_phandle(dev->of_node,
- "samsung,power-domain", 0);
- if (!pd_args.np)
- return -ENOENT;
- }
-
mutex_lock(&gpd_list_lock);
- pd = genpd_get_from_provider(&pd_args);
- of_node_put(pd_args.np);
+ pd = genpd_dev_pm_lookup(dev, 0);
if (IS_ERR(pd)) {
mutex_unlock(&gpd_list_lock);
- dev_dbg(dev, "%s() failed to find PM domain: %ld\n",
- __func__, PTR_ERR(pd));
return -EPROBE_DEFER;
}
- dev_dbg(dev, "adding to PM domain %s\n", pd->name);
-
- for (i = 1; i < GENPD_RETRY_MAX_MS; i <<= 1) {
- ret = genpd_add_device(pd, dev, NULL);
- if (ret != -EAGAIN)
-
[RFC PATCH 2/3] PM / Domains: Add support for devices with multiple domains
Some devices may require more than one PM domain to operate and this is
not currently by the PM domain framework. Furthermore, the current Linux
'device' structure only allows devices to be associated with a single PM
domain and so cannot easily be associated with more than one. To allow
devices to be associated with more than one PM domain, if multiple
domains are defined for a given device (eg. via device-tree), then:
1. Create a new PM domain for this device. The name of the new PM domain
created matches the device name for which it was created for.
2. Register the new PM domain as a sub-domain for all PM domains
required by the device.
3. Attach the device to the new PM domain.
By default the newly created PM domain is assumed to be in the 'off'
state to ensure that any parent PM domains will be turned on if not
already on when the new PM domain is turned on.
When a device associated with more than one PM domain is detached,
wait for any power-off work to complete, then remove the PM domain that
was created for the device by calling pm_genpd_remove() (this also
removes it as a child to any other PM domains) and free the memory
for the PM domain.
For devices using device-tree, devices that require multiple PM domains
are detected by seeing if the 'power-domains' property has more than one
entry defined.
Signed-off-by: Jon Hunter
---
Here is an example output from pm_genpd_summary following this change
for the Tegra210 XHCI device:
domain status slaves
/device runtime status
--
7009.usbon
/devices/platform/7009.usb unsupported
xusbc on 7009.usb
xusbb off-0
xusba on 7009.usb
I am not sure if this is confusing to have a device and domain with the
same name. So let me know if you have any thoughts!
drivers/base/power/domain.c | 102 ++--
1 file changed, 88 insertions(+), 14 deletions(-)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 382735949591..ee39824c03b3 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1826,7 +1826,7 @@ static void genpd_dev_pm_detach(struct device *dev, bool
power_off)
{
struct generic_pm_domain *pd;
unsigned int i;
- int ret = 0;
+ int count, ret = 0;
pd = genpd_lookup_dev(dev);
if (!pd)
@@ -1851,6 +1851,19 @@ static void genpd_dev_pm_detach(struct device *dev, bool
power_off)
/* Check if PM domain can be powered off after removing this device. */
genpd_queue_power_off_work(pd);
+
+ count = of_count_phandle_with_args(dev->of_node, "power-domains",
+ "#power-domain-cells");
+ if (count > 1) {
+ cancel_work_sync(&pd->power_off_work);
+
+ ret = pm_genpd_remove(pd);
+ if (ret < 0)
+ dev_err(dev, "failed to remove PM domain %s: %d\n",
+ pd->name, ret);
+
+ kfree(pd);
+ }
}
static void genpd_dev_pm_sync(struct device *dev)
@@ -1898,6 +1911,73 @@ static int genpd_dev_pm_attach_device(struct device *dev,
}
+static int genpd_dev_pm_attach_one(struct device *dev)
+{
+ struct generic_pm_domain *pd;
+ int ret;
+
+ mutex_lock(&gpd_list_lock);
+ pd = genpd_dev_pm_lookup(dev, 0);
+ if (IS_ERR(pd)) {
+ mutex_unlock(&gpd_list_lock);
+ return PTR_ERR(pd);
+ }
+
+ ret = genpd_dev_pm_attach_device(dev, pd);
+ mutex_unlock(&gpd_list_lock);
+
+ return ret;
+}
+
+static int genpd_dev_pm_attach_many(struct device *dev, unsigned int count)
+{
+ struct generic_pm_domain *pd, *parent;
+ unsigned int i;
+ int ret;
+
+ pd = kzalloc(sizeof(*pd), GFP_KERNEL);
+ if (!pd)
+ return -ENOMEM;
+
+ pd->name = dev_name(dev);
+
+ ret = pm_genpd_init(pd, NULL, true);
+ if (ret < 0)
+ goto err_free;
+
+ mutex_lock(&gpd_list_lock);
+ for (i = 0; i < count; i++) {
+ parent = genpd_dev_pm_lookup(dev, i);
+ if (IS_ERR(parent)) {
+ ret = PTR_ERR(parent);
+ goto err_remove;
+ }
+
+ ret = genpd_add_subdomain(parent, pd);
+ if (ret < 0)
+ goto err_remove;
+
+ }
+
+ ret = genpd_dev_pm_attach_device(dev, pd);
+ if (ret < 0)
+ goto err_remove;
+
+ mutex_unlock(&gpd_list_lock);
+
+ return ret;
+
+err_remove:
+ W
Re: [PATCH] ARM: tegra: nyan: Mark all USB ports as host
On 18/09/16 11:28, Paul Kocialkowski wrote:
> Nyan boards only have host USB ports (2 external, 1 internal), there is
> no OTG-enabled connector.
>
> Signed-off-by: Paul Kocialkowski
> ---
> arch/arm/boot/dts/tegra124-nyan.dtsi | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/tegra124-nyan.dtsi
> b/arch/arm/boot/dts/tegra124-nyan.dtsi
> index 18db797..254b2ee 100644
> --- a/arch/arm/boot/dts/tegra124-nyan.dtsi
> +++ b/arch/arm/boot/dts/tegra124-nyan.dtsi
> @@ -435,7 +435,7 @@
> usb2-0 {
> vbus-supply = <&vdd_usb1_vbus>;
> status = "okay";
> - mode = "otg";
> + mode = "host";
> };
>
> usb2-1 {
Sounds correct to me, so ...
Acked-by: Jon Hunter
Cheers
Jon
--
nvpublic
Re: [PATCH] ARM: tegra: nyan: Enable GPU node and related supply
On 18/09/16 15:13, Paul Kocialkowski wrote:
> This enables the GPU node for tegra124 nyan boards, which is required to
> get graphics acceleration with nouveau on these devices.
>
> Signed-off-by: Paul Kocialkowski
> ---
> arch/arm/boot/dts/tegra124-nyan.dtsi | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/tegra124-nyan.dtsi
> b/arch/arm/boot/dts/tegra124-nyan.dtsi
> index dab9509..225ca77 100644
> --- a/arch/arm/boot/dts/tegra124-nyan.dtsi
> +++ b/arch/arm/boot/dts/tegra124-nyan.dtsi
> @@ -42,6 +42,12 @@
> };
> };
>
> + gpu@0,5700 {
> + status = "okay";
> +
> + vdd-supply = <&vdd_gpu>;
> + };
> +
> serial@70006000 {
> /* Debug connector on the bottom of the board near SD card. */
> status = "okay";
> @@ -214,7 +220,7 @@
> regulator-always-on;
> };
>
> - sd6 {
> + vdd_gpu: sd6 {
> regulator-name = "+VDD_GPU_AP";
> regulator-min-microvolt = <65>;
> regulator-max-microvolt = <120>;
>
Looks good to me. I see the following error when booting but looking at the
code appears to be benign. Thierry, Alex, is this normal/okay?
[5.715181] nouveau 5700.gpu: NVIDIA GK20A (0ea000a1)
[5.720625] nouveau 5700.gpu: imem: using IOMMU
[5.803694] nouveau 5700.gpu: DRM: VRAM: 0 MiB
[5.808501] nouveau 5700.gpu: DRM: GART: 1048576 MiB
[5.816000] nouveau 5700.gpu: DRM: failed to create ce channel, -22
[5.924140] nouveau 5700.gpu: DRM: MM: using GRCE for buffer copies
Cheers
Jon
--
nvpublic
Re: [PATCH 1/4] ARM: tegra: nyan: Use proper IRQ type definitions
On 28/08/16 18:32, Paul Kocialkowski wrote:
> This switches a few interrupt definitions that were using
> GPIO_ACTIVE_HIGH as IRQ type, which is invalid.
May be you are right, but this does not describe why this is invalid.
Can you elaborate?
> Signed-off-by: Paul Kocialkowski
> ---
> arch/arm/boot/dts/tegra124-nyan.dtsi | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/boot/dts/tegra124-nyan.dtsi
> b/arch/arm/boot/dts/tegra124-nyan.dtsi
> index 271505e..30a77ec 100644
> --- a/arch/arm/boot/dts/tegra124-nyan.dtsi
> +++ b/arch/arm/boot/dts/tegra124-nyan.dtsi
> @@ -59,7 +59,7 @@
> compatible = "maxim,max98090";
> reg = <0x10>;
> interrupt-parent = <&gpio>;
> - interrupts = ;
> + interrupts = ;
If this in invalid then the DT binding doc for the max98090 should be
updated as well.
> };
>
> temperature-sensor@4c {
> @@ -325,7 +325,7 @@
> reg = <0x9>;
> interrupt-parent = <&gpio>;
> interrupts = - GPIO_ACTIVE_HIGH>;
> + IRQ_TYPE_EDGE_BOTH>;
> ti,ac-detect-gpios = <&gpio
> TEGRA_GPIO(J, 0)
> GPIO_ACTIVE_HIGH>;
>
Cheers
Jon
--
nvpublic
Re: [PATCH 2/4] ARM: tegra: nyan: Use external control for bq24735 charger
On 28/08/16 18:32, Paul Kocialkowski wrote: > Nyan boards come with an embedded controller that controls when to > enable and disable the charge. Thus, it should not be left up to the > kernel to handle that. > > Using the ti,external-control property allows specifying this use-case. So the bq24735 is populated under the EC's 'i2c-tunnel' property which is there to specifically interface it's child devices to the host. So I am a bit confused why this is expose to the host if it should not be used? Again you may right and I did find the original series [0] for this which specifically references the Acer Chromebook that needs this. However, I am not sure why this was never populated? Is there any other history here? What is the actual problem you see without making this change? The original series states ... "On Acer Chromebook 13 (CB5-311) this module fails to load if the charger is not inserted, and will error when it is removed." Cheers Jon [0] http://marc.info/?l=linux-pm&m=145447948705686&w=2 -- nvpublic
Re: [PATCH 3/4] ARM: tegra: nyan-big: Include compatible revisions for proper detection
On 28/08/16 18:32, Paul Kocialkowski wrote: > Depthcharge (the payload used with cros devices) will attempt to detect > boards using their revision. This includes all the known revisions for > the nyan-big board so that the dtb can be selected preferably. May be I am missing something here, but for the mainline there is only one dtb available and so why is this needed for the mainline? I understand for the downstream kernels that the chromebooks ship with they may include multiple dtbs, but for the mainline we only have one. Cheers Jon -- nvpublic
Re: [PATCH 4/4] ARM: tegra: nyan-blaze: Include compatible revisions for proper detection
On 28/08/16 18:32, Paul Kocialkowski wrote: > Depthcharge (the payload used with cros devices) will attempt to detect > boards using their revision. This includes all the known revisions for > the nyan-blaze board so that the dtb can be selected preferably. Again I am not sure what the benefit/need for this is. Jon -- nvpublic
Re: [RFC PATCH 2/3] PM / Domains: Add support for devices with multiple domains
On 20/09/16 11:28, Jon Hunter wrote:
> Some devices may require more than one PM domain to operate and this is
> not currently by the PM domain framework. Furthermore, the current Linux
> 'device' structure only allows devices to be associated with a single PM
> domain and so cannot easily be associated with more than one. To allow
> devices to be associated with more than one PM domain, if multiple
> domains are defined for a given device (eg. via device-tree), then:
> 1. Create a new PM domain for this device. The name of the new PM domain
>created matches the device name for which it was created for.
> 2. Register the new PM domain as a sub-domain for all PM domains
>required by the device.
> 3. Attach the device to the new PM domain.
>
> By default the newly created PM domain is assumed to be in the 'off'
> state to ensure that any parent PM domains will be turned on if not
> already on when the new PM domain is turned on.
>
> When a device associated with more than one PM domain is detached,
> wait for any power-off work to complete, then remove the PM domain that
> was created for the device by calling pm_genpd_remove() (this also
> removes it as a child to any other PM domains) and free the memory
> for the PM domain.
>
> For devices using device-tree, devices that require multiple PM domains
> are detected by seeing if the 'power-domains' property has more than one
> entry defined.
>
> Signed-off-by: Jon Hunter
> ---
>
> Here is an example output from pm_genpd_summary following this change
> for the Tegra210 XHCI device:
>
> domain status slaves
> /device runtime status
> --
> 7009.usbon
> /devices/platform/7009.usb unsupported
> xusbc on 7009.usb
> xusbb off-0
> xusba on 7009.usb
>
> I am not sure if this is confusing to have a device and domain with the
> same name. So let me know if you have any thoughts!
>
>
> drivers/base/power/domain.c | 102
> ++--
> 1 file changed, 88 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 382735949591..ee39824c03b3 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1826,7 +1826,7 @@ static void genpd_dev_pm_detach(struct device *dev,
> bool power_off)
> {
> struct generic_pm_domain *pd;
> unsigned int i;
> - int ret = 0;
> + int count, ret = 0;
>
> pd = genpd_lookup_dev(dev);
> if (!pd)
> @@ -1851,6 +1851,19 @@ static void genpd_dev_pm_detach(struct device *dev,
> bool power_off)
>
> /* Check if PM domain can be powered off after removing this device. */
> genpd_queue_power_off_work(pd);
> +
> + count = of_count_phandle_with_args(dev->of_node, "power-domains",
> +"#power-domain-cells");
> + if (count > 1) {
> + cancel_work_sync(&pd->power_off_work);
> +
> + ret = pm_genpd_remove(pd);
> + if (ret < 0)
> + dev_err(dev, "failed to remove PM domain %s: %d\n",
> + pd->name, ret);
> +
> + kfree(pd);
I realise I am missing a return if pm_genpd_remove() returns a failure!
Will correct this.
Jon
--
nvpublic
Re: [PATCH 3/4] ARM: tegra: nyan-big: Include compatible revisions for proper detection
On 20/09/16 18:53, Paul Kocialkowski wrote: > * PGP Signed by an unknown key > > Le mardi 20 septembre 2016 à 18:41 +0100, Jon Hunter a écrit : >> On 28/08/16 18:32, Paul Kocialkowski wrote: >>> >>> Depthcharge (the payload used with cros devices) will attempt to detect >>> boards using their revision. This includes all the known revisions for >>> the nyan-big board so that the dtb can be selected preferably. >> >> May be I am missing something here, but for the mainline there is only >> one dtb available and so why is this needed for the mainline? > > There is indeed a single dts in mainline, but depthcharge will use the > revision > to match the compatible string (e.g. it will look for google,nyan-big-rev5, > not > google,nyan-big), so we need to list them all in that single dts. Otherwise, > depthcharge will fall back to the default config, which may or may not be > suitable for nyan. Is tegra124-nyan-big.dtb not the default? Jon -- nvpublic
Re: [PATCH 2/4] ARM: tegra: nyan: Use external control for bq24735 charger
On 20/09/16 19:02, Paul Kocialkowski wrote: > * PGP Signed by an unknown key > > Le mardi 20 septembre 2016 à 18:40 +0100, Jon Hunter a écrit : >> On 28/08/16 18:32, Paul Kocialkowski wrote: >>> >>> Nyan boards come with an embedded controller that controls when to >>> enable and disable the charge. Thus, it should not be left up to the >>> kernel to handle that. >>> >>> Using the ti,external-control property allows specifying this use-case. >> >> So the bq24735 is populated under the EC's 'i2c-tunnel' property which >> is there to specifically interface it's child devices to the host. So I >> am a bit confused why this is expose to the host if it should not be used? > > Well, it needs to access the information in the read-only registers provided > by > the chip, which is allowed by the setup in place that you described. Is this to expose the current state to the kernel so we can monitor the battery state? > However, the EC has its internal state machine that decides when to start > charging, etc and so should be the only one to write registers, to avoid > conflicts. > >> Again you may right and I did find the original series [0] for this >> which specifically references the Acer Chromebook that needs this. >> However, I am not sure why this was never populated? Is there any other >> history here? > > I am also confused about why it wasn't applied earlier. However, the cros > kernel > is using the very same scheme. Do you have a reference? >> What is the actual problem you see without making this change? > > There is a risk of conflict (even though it's probably not that significant), > given the low variety of possible cases here. The idea is simply to say that > the > EC is in charge and to let it do its job without interfering. > >> The original series states ... >> >> "On Acer Chromebook 13 (CB5-311) this module fails to load if the >> charger is not inserted, and will error when it is removed." > > I'm confused about that comment. At this point (and with this patch), it works > normally. Ok, I think Thierry prefers to only apply fixes for problems that can be reproduced. Is there a simple way to check the battery status and charging status via say the sysfs? If I can test that this has no negative impact may be it is ok. Cheers Jon -- nvpublic
Re: [PATCH 3/4] ARM: tegra: nyan-big: Include compatible revisions for proper detection
On 20/09/16 19:02, Paul Kocialkowski wrote: > * PGP Signed by an unknown key > > Le mardi 20 septembre 2016 à 18:56 +0100, Jon Hunter a écrit : >> On 20/09/16 18:53, Paul Kocialkowski wrote: >>> >>>> Old Signed by an unknown key >>> >>> Le mardi 20 septembre 2016 à 18:41 +0100, Jon Hunter a écrit : >>>> >>>> On 28/08/16 18:32, Paul Kocialkowski wrote: >>>>> >>>>> >>>>> Depthcharge (the payload used with cros devices) will attempt to detect >>>>> boards using their revision. This includes all the known revisions for >>>>> the nyan-big board so that the dtb can be selected preferably. >>>> >>>> May be I am missing something here, but for the mainline there is only >>>> one dtb available and so why is this needed for the mainline? >>> >>> There is indeed a single dts in mainline, but depthcharge will use the >>> revision >>> to match the compatible string (e.g. it will look for google,nyan-big-rev5, >>> not >>> google,nyan-big), so we need to list them all in that single dts. Otherwise, >>> depthcharge will fall back to the default config, which may or may not be >>> suitable for nyan. >> >> Is tegra124-nyan-big.dtb not the default? > > You can't expect that to always be the case. The image format allows many > different dts to be provided, so I could easily build with multi_v7_defconfig > and have various dts for various devices in the same image, and just select a > random one as default. Really? Sounds odd. I was hoping that tegra124-nyan-big.dtb would be a catch all. > Here, default is really a fallback, the right one is expected to be detected > by > this mechanism. And it really doesn't hurt to provide that information for > proper detection. > > Note that this is done with many other cros devices in mainline (such as > rk3288 > veyrons). Yes, I guess the same is true for the tegra210-smaug and we do define all the revs for that one. May be extend the changelog comment to say what may happen without this change and the problem that this is fixing. Jon -- nvpublic
Re: [PATCH 1/4] ARM: tegra: nyan: Use proper IRQ type definitions
On 20/09/16 19:14, Paul Kocialkowski wrote: > * PGP Signed by an unknown key > > Le mardi 20 septembre 2016 à 18:15 +0100, Jon Hunter a écrit : >> On 28/08/16 18:32, Paul Kocialkowski wrote: >>> >>> This switches a few interrupt definitions that were using >>> GPIO_ACTIVE_HIGH as IRQ type, which is invalid. >> >> May be you are right, but this does not describe why this is invalid. >> Can you elaborate? > > GPIO_ACTIVE_HIGH is simply not the right kind of define to use in the > "interrupts" devicetree property. Values provided there are understood as > IRQ_TYPE_ defines. Right, but you are changing the type as GPIO_ACTIVE_HIGH = 0 and IRQ_TYPE_EDGE_FALLING = 2 and there is no comment about why this has been changed. It might be correct, but you need to explain it. Jon -- nvpublic
Re: [PATCH] ARM: tegra: nyan: Enable GPU node and related supply
On 20/09/16 19:17, Paul Kocialkowski wrote:
> * PGP Signed by an unknown key
>
> Le mardi 20 septembre 2016 à 13:24 +0100, Jon Hunter a écrit :
>> On 18/09/16 15:13, Paul Kocialkowski wrote:
>>>
>>> This enables the GPU node for tegra124 nyan boards, which is required to
>>> get graphics acceleration with nouveau on these devices.
>>>
>>> Signed-off-by: Paul Kocialkowski
>>> ---
>>> arch/arm/boot/dts/tegra124-nyan.dtsi | 8 +++-
>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/boot/dts/tegra124-nyan.dtsi
>>> b/arch/arm/boot/dts/tegra124-nyan.dtsi
>>> index dab9509..225ca77 100644
>>> --- a/arch/arm/boot/dts/tegra124-nyan.dtsi
>>> +++ b/arch/arm/boot/dts/tegra124-nyan.dtsi
>>> @@ -42,6 +42,12 @@
>>> };
>>> };
>>>
>>> + gpu@0,5700 {
>>> + status = "okay";
>>> +
>>> + vdd-supply = <&vdd_gpu>;
>>> + };
>>> +
>>> serial@70006000 {
>>> /* Debug connector on the bottom of the board near SD card.
>>> */
>>> status = "okay";
>>> @@ -214,7 +220,7 @@
>>> regulator-always-on;
>>> };
>>>
>>> - sd6 {
>>> + vdd_gpu: sd6 {
>>> regulator-name = "+VDD_GPU_AP";
>>> regulator-min-microvolt = <65>;
>>> regulator-max-microvolt =
>>> <120>;
>>>
>>
>> Looks good to me. I see the following error when booting but looking at the
>> code appears to be benign. Thierry, Alex, is this normal/okay?
>
> I have the same messages and asked Alexandre about them the other day. He told
> me that it looks normal.
Ok great. Hopefully, Alex can ACK then.
Cheers
Jon
--
nvpublic
Re: [PATCH 1/4] ARM: tegra: nyan: Use proper IRQ type definitions
On 21/09/16 09:26, Paul Kocialkowski wrote: > * PGP Signed by an unknown key > > Le mercredi 21 septembre 2016 à 08:52 +0100, Jon Hunter a écrit : >> On 20/09/16 19:14, Paul Kocialkowski wrote: >>> >>>> Old Signed by an unknown key >>> >>> Le mardi 20 septembre 2016 à 18:15 +0100, Jon Hunter a écrit : >>>> >>>> On 28/08/16 18:32, Paul Kocialkowski wrote: >>>>> >>>>> >>>>> This switches a few interrupt definitions that were using >>>>> GPIO_ACTIVE_HIGH as IRQ type, which is invalid. >>>> >>>> May be you are right, but this does not describe why this is invalid. >>>> Can you elaborate? >>> >>> GPIO_ACTIVE_HIGH is simply not the right kind of define to use in the >>> "interrupts" devicetree property. Values provided there are understood as >>> IRQ_TYPE_ defines. >> >> Right, but you are changing the type as GPIO_ACTIVE_HIGH = 0 and >> IRQ_TYPE_EDGE_FALLING = 2 and there is no comment about why this has >> been changed. It might be correct, but you need to explain it. > > This actually makes the IRQ trigger values consistent with the drivers, that > define them regardless of devicetree anyway. The max98090 driver > has IRQF_TRIGGER_FALLING and bq24735 has IRQF_TRIGGER_RISING | > IRQF_TRIGGER_FALLING. > > This is really more of a cosmetic change, it doesn't impact actual use. So you are saying that the drivers don't actually use the DT types? May be that is ok, and yes this is cosmetic, but this should be stated in the changelog as it is not clear what is going on here. Cheers Jon -- nvpublic
Re: [PATCH 3/4] ARM: tegra: nyan-big: Include compatible revisions for proper detection
On 21/09/16 08:43, Paul Kocialkowski wrote: ... >>> Depthcharge (the payload used with cros devices) will attempt to >>> detect >>> boards using their revision. This includes all the known revisions >>> for >>> the nyan-big board so that the dtb can be selected preferably. >> >> May be I am missing something here, but for the mainline there is only >> one dtb available and so why is this needed for the mainline? > > There is indeed a single dts in mainline, but depthcharge will use the > revision > to match the compatible string (e.g. it will look for google,nyan-big- > rev5, > not > google,nyan-big), so we need to list them all in that single dts. > Otherwise, > depthcharge will fall back to the default config, which may or may not > be > suitable for nyan. Is tegra124-nyan-big.dtb not the default? >>> >>> You can't expect that to always be the case. The image format allows many >>> different dts to be provided, so I could easily build with >>> multi_v7_defconfig >>> and have various dts for various devices in the same image, and just select >>> a >>> random one as default. >> >> Really? Sounds odd. I was hoping that tegra124-nyan-big.dtb would be a >> catch all. I meant I was hoping that compatible = "google,nyan-big" would be the catchall not the dtb file name ;-) > Yes, the image format (FIT) allows specifying multiple dtb and zImage > combinations in the same image[0]. Yes I am aware of that. Typically, I have been testing using a FIT image with single zImage and dtb. Hence no problems. So are you wanting to create a FIT image to support multiple boards and use the single FIT image for all? If so then I can see why you want this. Again please describe the motivation for the changes in the changelog so it is clear why we are adding this. Cheers Jon -- nvpublic
Re: [RFC PATCH 2/3] PM / Domains: Add support for devices with multiple domains
Hi Geert,
On 21/09/16 09:53, Geert Uytterhoeven wrote:
> Hi Jon,
>
> On Tue, Sep 20, 2016 at 12:28 PM, Jon Hunter wrote:
>> Some devices may require more than one PM domain to operate and this is
>> not currently by the PM domain framework. Furthermore, the current Linux
>> 'device' structure only allows devices to be associated with a single PM
>> domain and so cannot easily be associated with more than one. To allow
>> devices to be associated with more than one PM domain, if multiple
>> domains are defined for a given device (eg. via device-tree), then:
>> 1. Create a new PM domain for this device. The name of the new PM domain
>>created matches the device name for which it was created for.
>> 2. Register the new PM domain as a sub-domain for all PM domains
>>required by the device.
>> 3. Attach the device to the new PM domain.
>
> This looks a suboptimal to me: if you have n devices sharing the same PM
> domains, you would add n new subdomains?
Yes you would and so in that case it would appear to be suboptimal, I
agree. One option would be to name the new PM domain after its parents
and then see if any PM domains exist that matches the name and verify it
has the required parents. We could even add a prefix to the name to
indicate that this is a PM domain added by the core. Only problem is we
could get some long names!
> Having a clean way to specify multiple PM domains is very useful, though.
>
> E.g. on Renesas ARM SoCs, devices are usually part of two PM domains:
> 1. A power area (can be "always-on",
> 2. The clock domain.
>
> As power areas and clock domains are fairly orthogonal (the former use the
> .power_{off,on}() callbacks, the latter set GENPD_FLAG_PM_CLK and use the
> {at,de}tach_dev() callbacks), we currently setup both in the same driver
> (SYSC, for controlling power areas), which forwards the clock domain
> operations
> to the clock driver (CPG/MSTP or CPG/MSSR).
> Hence we have only single references in the power-domains properties, but
> having two would allow to drop the hardcoded links between the two drivers.
Glad to see others could benefit from this ...
> (Oh no, more DT backwards compatibility issues if this is accepted ;-)
... despite DT compat issues :-(
Jon
--
nvpublic
Re: [PATCH 2/4] ARM: tegra: nyan: Use external control for bq24735 charger
On 21/09/16 08:56, Paul Kocialkowski wrote: ... > Sure, this is exported at: /sys/class/power_supply/bq24735@5-0009 > Also, note that the power-supply next branch[2] has some more fixes for the > bq24735 driver. I tested the bq24735 on next-20160919 without any of your changes and when connecting or disconnecting the charger I see the following panic. Do you see this? Jon / # [ 30.120384] Unable to handle kernel NULL pointer dereference at virtual address 0004 [ 30.128489] pgd = c0004000 [ 30.131187] [0004] *pgd= [ 30.134759] Internal error: Oops: 17 [#1] PREEMPT SMP ARM [ 30.140141] Modules linked in: [ 30.143192] CPU: 1 PID: 71 Comm: kworker/1:1 Not tainted 4.8.0-rc6-next-20160919-2-gbc9771827865-dirty #574 [ 30.153254] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree) [ 30.159509] Workqueue: events power_supply_changed_work [ 30.164723] task: ee19f880 task.stack: ee1a [ 30.169239] PC is at sbs_external_power_changed+0x50/0x5c [ 30.174624] LR is at mod_timer+0x194/0x270 [ 30.178705] pc : []lr : []psr: 6113 [ 30.178705] sp : ee1a1eb8 ip : 8000 fp : ee8e3680 [ 30.190155] r10: eef98580 r9 : r8 : [ 30.195363] r7 : ee303000 r6 : c05afef8 r5 : ee3e03f8 r4 : ee3e03d0 [ 30.201872] r3 : r2 : r1 : 4193 r0 : 0001 [ 30.208382] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none [ 30.215497] Control: 10c5387d Table: adcf006a DAC: 0051 [ 30.221226] Process kworker/1:1 (pid: 71, stack limit = 0xee1a0210) [ 30.227474] Stack: (0xee1a1eb8 to 0xee1a2000) [ 30.231816] 1ea0: edc0bc00 ee303000 [ 30.239973] 1ec0: c05afef8 c05aff2c edc0bc20 c045ec1c eef95c80 eef95c80 0001 eea82d5c [ 30.248136] 1ee0: edc0bd98 ee3031c0 ee303218 c0eab740 c05afcc4 ee8e3680 ee3031c0 [ 30.256296] 1f00: eef9b800 eef98580 c0135824 eef98598 c0e02100 eef98580 ee8e3698 [ 30.264453] 1f20: 0008 eef98598 c0e02100 ee1a eef98580 c0135a74 ee840f80 ee8e3680 [ 30.272610] 1f40: c0135a3c ee840f80 ee8e3680 c0135a3c [ 30.280766] 1f60: c013af18 ee8e3680 [ 30.288922] 1f80: ee1a1f80 ee1a1f80 ee1a1f90 ee1a1f90 ee1a1fac ee840f80 [ 30.297078] 1fa0: c013ae3c c01078b8 [ 30.305234] 1fc0: [ 30.313389] 1fe0: 0013 [ 30.321555] [] (sbs_external_power_changed) from [] (__power_supply_changed_work+0x34/0x3c) [ 30.331623] [] (__power_supply_changed_work) from [] (class_for_each_device+0x4c/0xb4) [ 30.341254] [] (class_for_each_device) from [] (power_supply_changed_work+0x5c/0xb0) [ 30.350713] [] (power_supply_changed_work) from [] (process_one_work+0x124/0x33c) [ 30.359912] [] (process_one_work) from [] (worker_thread+0x38/0x4d4) [ 30.367983] [] (worker_thread) from [] (kthread+0xdc/0xf4) [ 30.375187] [] (kthread) from [] (ret_from_fork+0x14/0x3c) [ 30.382391] Code: e5911000 e3a4 ebee0b88 e5943008 (e5933004) [ 30.388504] ---[ end trace 083d55597e9a2254 ]--- [ 30.393140] Unable to handle kernel paging request at virtual address ffec [ 30.400342] pgd = c0004000 [ 30.403038] [ffec] *pgd=afffd861, *pte=, *ppte= [ 30.409309] Internal error: Oops: 37 [#2] PREEMPT SMP ARM [ 30.414690] Modules linked in: [ 30.417738] CPU: 1 PID: 71 Comm: kworker/1:1 Tainted: G D 4.8.0-rc6-next-20160919-2-gbc9771827865-dirty #574 [ 30.429013] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree) [ 30.435265] task: ee19f880 task.stack: ee1a [ 30.439782] PC is at kthread_data+0x4/0xc [ 30.443779] LR is at wq_worker_sleeping+0x8/0xc8 [ 30.448381] pc : []lr : []psr: 2193 [ 30.448381] sp : ee1a1c58 ip : eef98f28 fp : ee1a1cb4 [ 30.459836] r10: eef98a00 r9 : c0e5f000 r8 : c0d9ea00 [ 30.465048] r7 : ee19fcc8 r6 : c0e02e08 r5 : ee19f880 r4 : eef98a00 [ 30.471556] r3 : r2 : 0020 r1 : r0 : ee19f880 [ 30.478066] Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment none [ 30.485268] Control: 10c5387d Table: adcf006a DAC: 0051 [ 30.490998] Process kworker/1:1 (pid: 71, stack limit = 0xee1a0210) [ 30.497247] Stack: (0xee1a1c58 to 0xee1a2000) [ 30.501590] 1c40: eef98a00 c0833a68 [ 30.509747] 1c60: 0001 ee117a44 c0d99280 a193 c0833d4c 0001 ee117a44 [ 30.517904] 1c80: ee117540 ee19f880 c01faf3c ee1a ee1a18ec 0001 [ 30.526060] 1ca0: ee1a1cc8 ee19fc40 c05b0c5c 0001 ee1a1cc4 c0833d4c ee19f880 ee1a18ec [ 30.534217] 1cc0: ee85d3c0 c012277c ee1a1cc8 ee1a1cc8 00
Re: [RFC PATCH 2/3] PM / Domains: Add support for devices with multiple domains
Hi Geert, On 21/09/16 09:53, Geert Uytterhoeven wrote: > Hi Jon, > > On Tue, Sep 20, 2016 at 12:28 PM, Jon Hunter wrote: >> Some devices may require more than one PM domain to operate and this is >> not currently by the PM domain framework. Furthermore, the current Linux >> 'device' structure only allows devices to be associated with a single PM >> domain and so cannot easily be associated with more than one. To allow >> devices to be associated with more than one PM domain, if multiple >> domains are defined for a given device (eg. via device-tree), then: >> 1. Create a new PM domain for this device. The name of the new PM domain >>created matches the device name for which it was created for. >> 2. Register the new PM domain as a sub-domain for all PM domains >>required by the device. >> 3. Attach the device to the new PM domain. > > This looks a suboptimal to me: if you have n devices sharing the same PM > domains, you would add n new subdomains? BTW, would this be the case today for some renesas devices or are you just pointing this out as something that could be optimised/improved? Cheers Jon -- nvpublic
[PATCH] of/irq: Mark initialised interrupt controllers as populated
For interrupt controllers successfully initialised early via device-tree, mark these interrupt controllers as populated so we don't unnecessarily create a device and populate any platform data later on in the boot sequence when we populate all the various platform devices. Signed-off-by: Jon Hunter --- drivers/of/irq.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/of/irq.c b/drivers/of/irq.c index 6ec743faabe8..1b58cd574316 100644 --- a/drivers/of/irq.c +++ b/drivers/of/irq.c @@ -557,6 +557,8 @@ void __init of_irq_init(const struct of_device_id *matches) * its children can get processed in a subsequent pass. */ list_add_tail(&desc->list, &intc_parent_list); + + of_node_set_flag(desc->dev, OF_POPULATED); } /* Get the next pending parent that might have children */ -- 2.1.4
[RFC PATCH] irqdomain: Fix disposal of mappings for interrupt hierarchies
The function irq_create_of_mapping() is used to create an interrupt
mapping. However, depending on whether the irqdomain, to which the
interrupt belongs, is part of a hierarchy, determines whether the
mapping is created via calling irq_domain_alloc_irqs() or
irq_create_mapping().
To dispose of the interrupt mapping, drivers call irq_dispose_mapping().
However, this function does not check to see if the irqdomain is part
of a hierarchy or not and simply assumes that it was mapped via calling
irq_create_mapping() so calls irq_domain_disassociate() to unmap the
interrupt.
Fix this by checking to see if the irqdomain is part of a hierarchy and
if so call irq_domain_free_irqs() to free/unmap the interrupt.
Signed-off-by: Jon Hunter
---
Please note that I have not observed issues with this, but something
that did not make sense to me from looking at the code.
kernel/irq/irqdomain.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index caa6a63d26f0..5d89d724a02a 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -680,8 +680,12 @@ void irq_dispose_mapping(unsigned int virq)
if (WARN_ON(domain == NULL))
return;
- irq_domain_disassociate(domain, virq);
- irq_free_desc(virq);
+ if (irq_domain_is_hierarchy(domain)) {
+ irq_domain_free_irqs(virq, 1);
+ } else {
+ irq_domain_disassociate(domain, virq);
+ irq_free_desc(virq);
+ }
}
EXPORT_SYMBOL_GPL(irq_dispose_mapping);
--
2.1.4
[PATCH] PM / clk: Add support for adding a specific clock from device-tree
Some drivers using the PM clocks framework need to add specific clocks
from device-tree using a name by calling the functions
of_clk_get_by_name() and then pm_clk_add_clk(). Rather than having
drivers call both functions, add a helper function of_pm_clk_add_clk()
that will call these functions so drivers can call a single function
to add a specific clock from device-tree.
Signed-off-by: Jon Hunter
---
Some examples of drivers that would be able to make use of this new
function are drivers/dma/tegra210-adma.c and
drivers/irqchip/irq-gic-pm.c.
drivers/base/power/clock_ops.c | 32
include/linux/pm_clock.h | 1 +
2 files changed, 33 insertions(+)
diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
index 761f5c21f9f0..8e2e4757adcb 100644
--- a/drivers/base/power/clock_ops.c
+++ b/drivers/base/power/clock_ops.c
@@ -141,6 +141,38 @@ EXPORT_SYMBOL_GPL(pm_clk_add_clk);
/**
+ * of_pm_clk_add_clk - Start using a device clock for power management.
+ * @dev: Device whose clock is going to be used for power management.
+ * @name: Name of clock that is going to be used for power management.
+ *
+ * Add the clock described in the 'clocks' device-tree node that matches
+ * with the 'name' provided, to the list of clocks used for the power
+ * management of @dev. On success, returns 0. Returns a negative error
+ * code if the clock is not found or cannot be added.
+ */
+int of_pm_clk_add_clk(struct device *dev, const char *name)
+{
+ struct clk *clk;
+ int ret;
+
+ if (!dev || !dev->of_node || !name)
+ return -EINVAL;
+
+ clk = of_clk_get_by_name(dev->of_node, name);
+ if (IS_ERR(clk))
+ return PTR_ERR(clk);
+
+ ret = pm_clk_add_clk(dev, clk);
+ if (ret) {
+ clk_put(clk);
+ return ret;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(of_pm_clk_add_clk);
+
+/**
* of_pm_clk_add_clks - Start using device clock(s) for power management.
* @dev: Device whose clock(s) is going to be used for power management.
*
diff --git a/include/linux/pm_clock.h b/include/linux/pm_clock.h
index 308d6044f153..09779b0ae720 100644
--- a/include/linux/pm_clock.h
+++ b/include/linux/pm_clock.h
@@ -42,6 +42,7 @@ extern int pm_clk_create(struct device *dev);
extern void pm_clk_destroy(struct device *dev);
extern int pm_clk_add(struct device *dev, const char *con_id);
extern int pm_clk_add_clk(struct device *dev, struct clk *clk);
+extern int of_pm_clk_add_clk(struct device *dev, const char *name);
extern int of_pm_clk_add_clks(struct device *dev);
extern void pm_clk_remove(struct device *dev, const char *con_id);
extern void pm_clk_remove_clk(struct device *dev, struct clk *clk);
--
2.1.4
Re: [PATCH 7/8] dmaengine: tegra20-apb-dma: Only calculate residue if txstate exists.
On 21/06/16 17:01, Vinod Koul wrote: > On Wed, Jun 08, 2016 at 09:51:57AM +0100, Jon Hunter wrote: >> Hi Peter, >> >> On 07/06/16 18:38, Peter Griffin wrote: >>> There is no point calculating the residue if there is >>> no txstate to store the value. >>> >>> Signed-off-by: Peter Griffin >>> --- >>> drivers/dma/tegra20-apb-dma.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c >>> index 01e316f..7f4af8c 100644 >>> --- a/drivers/dma/tegra20-apb-dma.c >>> +++ b/drivers/dma/tegra20-apb-dma.c >>> @@ -814,7 +814,7 @@ static enum dma_status tegra_dma_tx_status(struct >>> dma_chan *dc, >>> unsigned int residual; >>> >>> ret = dma_cookie_status(dc, cookie, txstate); >>> - if (ret == DMA_COMPLETE) >>> + if (ret == DMA_COMPLETE || !txstate) >>> return ret; >> >> Thanks for reporting this. I agree that we should not do this, however, >> looking at the code for Tegra, I am wondering if this could change the >> actual state that is returned. Looking at dma_cookie_status() it will >> call dma_async_is_complete() which will return either DMA_COMPLETE or >> DMA_IN_PROGRESS. It could be possible that the actual state for the >> DMA transfer in the tegra driver is DMA_ERROR, so I am wondering if we >> should do something like the following ... > > This one is stopping code execution when residue is not valid. Do notice > that it check for DMA_COMPLETE OR txstate. In other cases, wit will return > 'that' state when txstate is NULL. Sorry what do you mean by "this one"? My point is that if the status is not DMA_COMPLETE, then it is possible that it could be DMA_ERROR (for tegra that is). However, dma_cookie_status will only return DMA_IN_PROGRESS or DMA_COMPLETE and so if 'txstate' is NULL we will not see the DMA_ERROR status anymore and just think it is in progress when it is actually an error. I do agree that the driver is broken as we are not checking for !txstate, but this also changes the behaviour a bit. Cheers Jon -- nvpublic
Re: [RFC PATCH 2/3] PM / Domains: Add support for devices with multiple domains
Hi Geert, On 21/09/16 15:57, Geert Uytterhoeven wrote: > Hi Jon, > > On Wed, Sep 21, 2016 at 4:37 PM, Jon Hunter wrote: >> On 21/09/16 09:53, Geert Uytterhoeven wrote: >>> On Tue, Sep 20, 2016 at 12:28 PM, Jon Hunter wrote: >>>> Some devices may require more than one PM domain to operate and this is >>>> not currently by the PM domain framework. Furthermore, the current Linux >>>> 'device' structure only allows devices to be associated with a single PM >>>> domain and so cannot easily be associated with more than one. To allow >>>> devices to be associated with more than one PM domain, if multiple >>>> domains are defined for a given device (eg. via device-tree), then: >>>> 1. Create a new PM domain for this device. The name of the new PM domain >>>>created matches the device name for which it was created for. >>>> 2. Register the new PM domain as a sub-domain for all PM domains >>>>required by the device. >>>> 3. Attach the device to the new PM domain. >>> >>> This looks a suboptimal to me: if you have n devices sharing the same PM >>> domains, you would add n new subdomains? >> >> BTW, would this be the case today for some renesas devices or are you >> just pointing this out as something that could be optimised/improved? > > This is the case for all Renesas SoCs that have power areas: devices belong > to both the PM domain for the power area, and to the PM domain for the clock > domain. To quantify this a bit, for the Renesas case, how many of these duplicated domains would there be if you were to use this approach as-is? I would like to see some agreement about whether we would allow the 'power-domains' property to have more than one power-domain. We could always improve the implementation in the future. I am quite happy to re-work this RFC to avoid duplicated domains for devices like Renesas if people are on board with the overall proposal. Cheers Jon -- nvpublic
Re: [PATCH V6 3/9] irqdomain: Don't set type when mapping an IRQ
On 29/07/16 04:53, Masahiro Yamada wrote: > Hi. > > I noticed my board would not work any more > when pulling recent updates. > > I did "git-bisect" and I found the following commit is it. > > commit 1e2a7d78499ec8859d2b469051b7b80bad3b08aa > Author: Jon Hunter > Date: Tue Jun 7 16:12:28 2016 +0100 > > irqdomain: Don't set type when mapping an IRQ > > With reverting it, everything works fine for me. > > My board is arch/arm64/boot/dts/socionext/uniphier-ph1-ld20-ref.dts > > Is anything wrong with IRQ settings in my Device Tree? Most likely. > If 1e2a7d784 is really a buggy commit, > could you do something please? Before this commit bad IRQ type settings in device-tree were not getting reported and so that's why most likely it is bad IRQ type settings. As Marc mentioned without any more details (which IRQ for which device is failing) we cannot confirm. So I would look at the failing IRQ which is now failing when being requested and see if the type in device-tree is correct. Cheers Jon -- nvpublic
Re: [Regression] "irqdomain: Don't set type when mapping an IRQ" breaks nexus7 gpio buttons
Hi John,
On 30/07/16 05:39, John Stultz wrote:
> Hey Jon,
> So after rebasing my nexus7 patch stack onto pre-4.8-rc1 tree, I
> noticed the power/volume buttons stopped working.
>
> I did a manual rebased bisection and chased it down to your commit
> 1e2a7d78499e ("irqdomain: Don't set type when mapping an IRQ").
>
> Reverting that patch makes things work again, so I wanted to see if
> there was any debugging info I could provide to try to help narrow
> down the problem here. (Sorry, I'd tinker myself with it some and try
> to debug the issue, but after burning my friday night on this, I'm
> eager to get away from the keyboard for the weekend).
Before this commit bad IRQ type settings in device-tree were not getting
reported and so failures to set the IRQ type were going unnoticed. It's
most likely a bad IRQ type settings somewhere.
As Thomas mentioned hopefully dmesg will shed a bit more light.
Otherwise it can be worth looking at the ->irq_set_type() function for
the irqchips in the path of the interrupt requested to see if any are
failing. Looking at the nexus7 (assuming qcom variant), it looks like
there are 3 irqchips in the path (pm8921 --> apq8064-pinctrl --> gic).
The pm8xxx_irq_set_type() could return a failure when setting up the IRQ
type and could be worth checking. It does not look like the set_type for
the apq8064-pinctrl should ever fail (apart from calling BUG() which
would be obvious). The gic can also return a failure for setting the
type, but I did not see anything at first glance that looks incorrect in
the dts.
If we can narrow down irqchip, then hopefully it will be clearer.
Cheers
Jon
--
nvpublic
Re: [PATCH v3 2/2] dmaengine: tegra210-adma: Add memcpy support
On 06/09/16 19:42, Nicolin Chen wrote: > ADMA supports non-flow controlled Memory-to-Memory direction > transactions. So this patch just adds an initial support for > that. It passed a simple dmatest: > echo dma1chan0 > /sys/module/dmatest/parameters/channel > echo 1024 > /sys/module/dmatest/parameters/iterations > echo 0 > /sys/module/dmatest/parameters/dmatest > echo 1 > /sys/module/dmatest/parameters/run > dmesg | grep dmatest > Started 1 threads using dma1chan0 > dma1chan0-copy0: summary 1024 tests, 0 failures 2054 iops 16520 KB/s (0) What board and kernel did you try this on? I have tried this on a tegra210-jetson-tx1 and I get : [ 202.569204] dmatest: Started 1 threads using dma1chan0 [ 205.620318] dmatest: dma1chan0-copy0: result #1: 'test timed out' with src_off=0x86c dst_off=0xc80 len=0x307c (0) [ 208.692315] dmatest: dma1chan0-copy0: result #2: 'test timed out' with src_off=0x3288 dst_off=0x2720 len=0xa1c (0) [ 211.764323] dmatest: dma1chan0-copy0: result #3: 'test timed out' with src_off=0x2f44 dst_off=0x3164 len=0x3a4 (0) [ 214.836309] dmatest: dma1chan0-copy0: result #4: 'test timed out' with src_off=0x17d0 dst_off=0x2b10 len=0xe2c (0) [ 217.908305] dmatest: dma1chan0-copy0: result #5: 'test timed out' with src_off=0x23d8 dst_off=0xe90 len=0xb7c (0) ... I also tried a tegra210-smaug and I get: [ 167.508828] dmatest: Started 1 threads using dma1chan0 [ 167.508870] dmatest: dma1chan0-copy0: dstbuf[0x0] not copied! Expected db, got 09 [ 167.508873] dmatest: dma1chan0-copy0: dstbuf[0x1] not copied! Expected da, got 05 [ 167.508875] dmatest: dma1chan0-copy0: dstbuf[0x2] not copied! Expected d9, got 26 [ 167.508876] dmatest: dma1chan0-copy0: dstbuf[0x3] not copied! Expected d8, got 16 ... I am testing with your patches on next-20160905. Cheers Jon -- nvpublic
Re: [PATCH v3 2/2] dmaengine: tegra210-adma: Add memcpy support
On 08/09/16 15:08, Jon Hunter wrote: > > On 06/09/16 19:42, Nicolin Chen wrote: >> ADMA supports non-flow controlled Memory-to-Memory direction >> transactions. So this patch just adds an initial support for >> that. It passed a simple dmatest: >> echo dma1chan0 > /sys/module/dmatest/parameters/channel >> echo 1024 > /sys/module/dmatest/parameters/iterations >> echo 0 > /sys/module/dmatest/parameters/dmatest >> echo 1 > /sys/module/dmatest/parameters/run >> dmesg | grep dmatest >> Started 1 threads using dma1chan0 >> dma1chan0-copy0: summary 1024 tests, 0 failures 2054 iops 16520 KB/s (0) > > What board and kernel did you try this on? > > I have tried this on a tegra210-jetson-tx1 and I get : > > [ 202.569204] dmatest: Started 1 threads using dma1chan0 > [ 205.620318] dmatest: dma1chan0-copy0: result #1: 'test timed out' with > src_off=0x86c dst_off=0xc80 len=0x307c (0) > [ 208.692315] dmatest: dma1chan0-copy0: result #2: 'test timed out' with > src_off=0x3288 dst_off=0x2720 len=0xa1c (0) > [ 211.764323] dmatest: dma1chan0-copy0: result #3: 'test timed out' with > src_off=0x2f44 dst_off=0x3164 len=0x3a4 (0) > [ 214.836309] dmatest: dma1chan0-copy0: result #4: 'test timed out' with > src_off=0x17d0 dst_off=0x2b10 len=0xe2c (0) > [ 217.908305] dmatest: dma1chan0-copy0: result #5: 'test timed out' with > src_off=0x23d8 dst_off=0xe90 len=0xb7c (0) > ... > > I also tried a tegra210-smaug and I get: > > [ 167.508828] dmatest: Started 1 threads using dma1chan0 > [ 167.508870] dmatest: dma1chan0-copy0: dstbuf[0x0] not copied! Expected db, > got 09 > [ 167.508873] dmatest: dma1chan0-copy0: dstbuf[0x1] not copied! Expected da, > got 05 > [ 167.508875] dmatest: dma1chan0-copy0: dstbuf[0x2] not copied! Expected d9, > got 26 > [ 167.508876] dmatest: dma1chan0-copy0: dstbuf[0x3] not copied! Expected d8, > got 16 > ... I tried this again on my audio testing branch for tegra210 and it worked ... [ 36.427210] dmatest: Started 1 threads using dma1chan0 [ 37.036948] dmatest: dma1chan0-copy0: summary 1024 tests, 0 failures 2410 iops 19419 KB/s (0) Do you have other patches in your branch? If so it would be good to understand the dependency. May be we are missing a clock somewhere? Cheers Jon -- nvpublic
Re: [PATCH] dmaengine: squash lines for immediate return
On 12/09/16 20:20, Masahiro Yamada wrote: > Remove unneeded variables and assignments. > > Signed-off-by: Masahiro Yamada > --- > > drivers/dma/cppi41.c | 30 +- > drivers/dma/imx-sdma.c| 15 --- > drivers/dma/ppc4xx/adma.c | 15 ++- > drivers/dma/tegra20-apb-dma.c | 4 +--- > 4 files changed, 16 insertions(+), 48 deletions(-) ... > diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c > index 6ab9eb9..89795a2 100644 > --- a/drivers/dma/tegra20-apb-dma.c > +++ b/drivers/dma/tegra20-apb-dma.c > @@ -337,9 +337,7 @@ static struct tegra_dma_sg_req *tegra_dma_sg_req_get( > } > spin_unlock_irqrestore(&tdc->lock, flags); > > - sg_req = kzalloc(sizeof(struct tegra_dma_sg_req), GFP_NOWAIT); > - > - return sg_req; > + return kzalloc(sizeof(struct tegra_dma_sg_req), GFP_NOWAIT); > } > > static int tegra_dma_slave_config(struct dma_chan *dc, For the Tegra bit ... Acked-by: Jon Hunter Cheers Jon -- nvpublic
Re: [PATCH v3 2/2] dmaengine: tegra210-adma: Add memcpy support
On 12/09/16 21:50, Nicolin Chen wrote: > On Mon, Sep 12, 2016 at 03:34:08PM +0100, Jon Hunter wrote: > >>> Sorry. I forgot to mention that the TEGRA210_CLK_APE_SLCG_OVR >>> clock is required for the tests. So I cherry-picked 2 patches >>> from your audio branch to the linux-next: >>> clk: tegra210: Add SLCG override gate clocks >>> ARM64: tegra: DT: Add SLCG clock for AUD > >>> And it seems that you've submitted that patch once but it got >>> hold because it wasn't so useful at that time? > >> Yes it was not being used at the time. It is on my list of things to do >> and we need to revisit it. There was some discussion on the best way to >> handle these clocks from a client perspective. I am not sure we came to >> a conclusion on this. I need to find some time to look at this. > > I may also take a look to speed it up. Yet, putting that clock > aside, how about this patch then? I think we don't need to wait > for that clock patch in order to announce that we support this > now on a specific SoC but can just treat it as a new feature of > a DMA controller, which sounds quite plausible to me since the > ADMA module is now being disabled in all dts files of existing > SoCs -- There have to be some local changes in any way so as to > test it with the mainline code. I am fine with the changes. However, I am wondering if we should sort out this clock business first just in case someone tries to use this. Cheers Jon -- nvpublic
Re: [PATCH] PM / Domains: Allow holes in genpd_data.domains array
On 15/09/16 11:39, Tomeu Vizoso wrote:
> In platforms such as Rockchip's, the array of domains isn't always
> filled without holes, as which domains are present depend on the
> particular SoC revision.
>
> By allowing holes to be in the array, such SoCs can still use a single
> set of constants to index the array of power domains.
>
> Fixes: 0159ec670763 ("PM / Domains: Verify the PM domain is present when
> adding a provider")
> Signed-off-by: Tomeu Vizoso
> Cc: Jon Hunter
> Cc: Heiko Stuebner
> ---
> drivers/base/power/domain.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index b0cf46dcae73..ce3f483ec67b 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1581,6 +1581,8 @@ int of_genpd_add_provider_onecell(struct device_node
> *np,
> mutex_lock(&gpd_list_lock);
>
> for (i = 0; i < data->num_domains; i++) {
> + if (!data->domains[i])
> + continue;
> if (!pm_genpd_present(data->domains[i]))
> goto error;
>
Sounds reasonable and sorry I had not thought of this. Can you also make
sure we check that !data->domains[i] in the error path as well or
potentially we could have a NULL pointer dereference in the case of an
actual error.
Cheers
Jon
--
nvpublic
Re: [PATCH v2] PM / Domains: Allow holes in genpd_data.domains array
On 15/09/16 13:05, Tomeu Vizoso wrote:
> In platforms such as Rockchip's, the array of domains isn't always
> filled without holes, as which domains are present depend on the
> particular SoC revision.
>
> By allowing holes to be in the array, such SoCs can still use a single
> set of constants to index the array of power domains.
>
> Fixes: 0159ec670763 ("PM / Domains: Verify the PM domain is present when
> adding a provider")
> Signed-off-by: Tomeu Vizoso
> Cc: Jon Hunter
> Cc: Heiko Stuebner
>
> ---
>
> v2: Also skip holes in the error path.
> ---
> drivers/base/power/domain.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index b0cf46dcae73..83ae3d7d3fdd 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1581,6 +1581,8 @@ int of_genpd_add_provider_onecell(struct device_node
> *np,
> mutex_lock(&gpd_list_lock);
>
> for (i = 0; i < data->num_domains; i++) {
> + if (!data->domains[i])
> + continue;
> if (!pm_genpd_present(data->domains[i]))
> goto error;
>
> @@ -1598,6 +1600,8 @@ int of_genpd_add_provider_onecell(struct device_node
> *np,
>
> error:
> while (i--) {
> + if (!data->domains[i])
> + continue;
> data->domains[i]->provider = NULL;
> data->domains[i]->has_provider = false;
> }
>
Thanks!
Acked-by: Jon Hunter
Cheers
Jon
--
nvpublic
Re: [Regression] "irqdomain: Don't set type when mapping an IRQ" breaks nexus7 gpio buttons
On 05/08/16 19:12, John Stultz wrote:
> On Sat, Jul 30, 2016 at 1:07 AM, Thomas Gleixner wrote:
>> On Fri, 29 Jul 2016, John Stultz wrote:
>>> Hey Jon,
>>> So after rebasing my nexus7 patch stack onto pre-4.8-rc1 tree, I
>>> noticed the power/volume buttons stopped working.
>>>
>>> I did a manual rebased bisection and chased it down to your commit
>>> 1e2a7d78499e ("irqdomain: Don't set type when mapping an IRQ").
>>>
>>> Reverting that patch makes things work again, so I wanted to see if
>>> there was any debugging info I could provide to try to help narrow
>>> down the problem here. (Sorry, I'd tinker myself with it some and try
>>> to debug the issue, but after burning my friday night on this, I'm
>>> eager to get away from the keyboard for the weekend).
>>
>> dmesg should contain debug output which yells about non matching types. Can
>> you provide that?
>
> I'm not seeing anything about problematic type matching in the dmesg.
>
> I've also added LinusW's patches but that didn't seem to help either.
>
> dmesg attached.
I am wondering if it is related to ...
[1.678160] apq8064-pinctrl 80.pinctrl: pin GPIO_16 already requested by
1654.serial; cannot claim for 1658.i2c
[1.678207] apq8064-pinctrl 80.pinctrl: pin-16 (1658.i2c) status -22
[1.678362] 1-0010 supply vcc33 not found, using dummy regulator
[1.678779] 1-0010 supply vccio not found, using dummy regulator
[1.702427] apq8064-pinctrl 80.pinctrl: could not request pin 16
(GPIO_16) from group gpio16 on device 80.pinctrl
Cheers
Jon
--
nvpublic
Re: [Regression] "irqdomain: Don't set type when mapping an IRQ" breaks nexus7 gpio buttons
On 06/08/16 00:45, John Stultz wrote:
> On Mon, Aug 1, 2016 at 3:26 AM, Jon Hunter wrote:
>> Hi John,
>>
>> On 30/07/16 05:39, John Stultz wrote:
>>> Hey Jon,
>>> So after rebasing my nexus7 patch stack onto pre-4.8-rc1 tree, I
>>> noticed the power/volume buttons stopped working.
>>>
>>> I did a manual rebased bisection and chased it down to your commit
>>> 1e2a7d78499e ("irqdomain: Don't set type when mapping an IRQ").
>>>
>>> Reverting that patch makes things work again, so I wanted to see if
>>> there was any debugging info I could provide to try to help narrow
>>> down the problem here. (Sorry, I'd tinker myself with it some and try
>>> to debug the issue, but after burning my friday night on this, I'm
>>> eager to get away from the keyboard for the weekend).
>>
>> Before this commit bad IRQ type settings in device-tree were not getting
>> reported and so failures to set the IRQ type were going unnoticed. It's
>> most likely a bad IRQ type settings somewhere.
>>
>> As Thomas mentioned hopefully dmesg will shed a bit more light.
>>
>> Otherwise it can be worth looking at the ->irq_set_type() function for
>> the irqchips in the path of the interrupt requested to see if any are
>> failing. Looking at the nexus7 (assuming qcom variant), it looks like
>> there are 3 irqchips in the path (pm8921 --> apq8064-pinctrl --> gic).
>> The pm8xxx_irq_set_type() could return a failure when setting up the IRQ
>> type and could be worth checking. It does not look like the set_type for
>> the apq8064-pinctrl should ever fail (apart from calling BUG() which
>> would be obvious). The gic can also return a failure for setting the
>> type, but I did not see anything at first glance that looks incorrect in
>> the dts.
>>
>> If we can narrow down irqchip, then hopefully it will be clearer.
>
> The pm_8xxx_irq_set_type doesn't seem to be failing as far as I can see..
>
> Looking at the patch that seems to cause the trouble, I narrowed it
> down to just the following chunk:
>
> @@ -614,7 +615,11 @@ unsigned int irq_create_fwspec_mapping(struct
> irq_fwspec *fwspec)
> * it now and return the interrupt number.
> */
> if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
> - irq_set_irq_type(virq, type);
> + irq_data = irq_get_irq_data(virq);
> + if (!irq_data)
> + return 0;
> +
> + irqd_set_trigger_type(irq_data, type);
> return virq;
> }
>
> If I revert just that, it works again.
>
> I was worried we were hitting an early failure from !irq_data, but it
> seems there's some subtle difference between irqd_set_trigger_type and
> irq_set_type that makes the former break for me.
Thanks this is good info and at the same time odd.
I am guessing that it is failing above because the irq_data is not found
for the irq?
What is odd, is that the above sequence is only executed if a irq
mapping exists and so really, AFAICT this should not happen. Ie. the irq
descriptor should have been allocated for the mapping to exist. We
should probably warn if this happens.
Without reverting the above, can you add a print to show the
domain->name, hwirq and virq information if !irq_data? That will confirm
the domain for us.
Also it could be worth enabling all the debug prints in
kernel/irq/irqdomain.c to see what is happening on boot.
Cheers
Jon
--
nvpublic
Re: [PATCH 0/6] Some Tegra I2C Updates
Hi Wolfram, On 11/08/16 11:16, Jon Hunter wrote: > Add runtime-pm and pinctrl support for Tegra I2C driver. > > The first 4 patches are trivial clean-up/simplification changes. > > Jon Hunter (6): > i2c: tegra: Add missing new line characters > i2c: tegra: Remove non device-tree support > i2c: tegra: Use device name for adapter name > i2c: tegra: Simplify I2C resume > i2c: tegra: Add runtime power-management support > i2c: tegra: Add pinctrl support > > drivers/i2c/busses/i2c-tegra.c | 97 > +++--- > 1 file changed, 63 insertions(+), 34 deletions(-) Any comments or can these be queued? Cheers Jon -- nvpublic
Re: [PATCH v2 3/6] dt/bindings: Add bindings for Tegra GMI controller
On 24/08/16 14:37, Mirza Krak wrote: > From: Mirza Krak > > Document the devicetree bindings for the Generic Memory Interface (GMI) > bus driver found on Tegra SOCs. > > Signed-off-by: Mirza Krak > --- > Changes in v2: > - Updated examples and some information based on comments from Jon Hunter. > > .../devicetree/bindings/bus/nvidia,tegra20-gmi.txt | 132 > + > 1 file changed, 132 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/bus/nvidia,tegra20-gmi.txt > > diff --git a/Documentation/devicetree/bindings/bus/nvidia,tegra20-gmi.txt > b/Documentation/devicetree/bindings/bus/nvidia,tegra20-gmi.txt > new file mode 100644 > index 000..8c1e15f > --- /dev/null > +++ b/Documentation/devicetree/bindings/bus/nvidia,tegra20-gmi.txt > @@ -0,0 +1,132 @@ > +Device tree bindings for NVIDIA Tegra Generic Memory Interface bus > + > +The Generic Memory Interface bus enables memory transfers between internal > and > +external memory. Can be used to attach various high speed devices such as > +synchronous/asynchronous NOR, FPGA, UARTS and more. > + > +The actual devices are instantiated from the child nodes of a GMI node. > + > +Required properties: > + - compatible : Should contain one of the following: > +For Tegra20 must contain "nvidia,tegra20-gmi". > +For Tegra30 must contain "nvidia,tegra30-gmi". > + - reg: Should contain GMI controller registers location and length. > + - clocks: Must contain an entry for each entry in clock-names. > + - clock-names: Must include the following entries: "gmi" > + - resets : Must contain an entry for each entry in reset-names. > + - reset-names : Must include the following entries: "gmi" > + - #address-cells: The number of cells used to represent physical base > + addresses in the GMI address space. Should be 1. > + - #size-cells: The number of cells used to represent the size of an address > + range in the GMI address space. Should be 1. > + - ranges: Must be set up to reflect the memory layout with three integer > values > + for each chip-select line in use (only one entry is supported, see below > + comments): > + > + > +Note that the GMI controller does not have any internal chip-select address > +decoding, because of that chip-selects either need to be managed via software > +or by employing external chip-select decoding logic. > + > +If external chip-select logic is used to support multiple devices it is > assumed > +that the devices use the same timing and so are probably the same type. It > also > +assumes that they can fit in the 256MB address range. In this case only one > +child device is supported which represents the active chip-select line, see > +examples for more insight. > + > +Required child cs node properties: > + - reg: First entry should contain the active chip-select number > + > +Optional child cs node properties: > + > + - nvidia,snor-data-width-32bit: Use 32bit data-bus, default is 16bit. > + - nvidia,snor-mux-mode: Enable address/data MUX mode. > + - nvidia,snor-rdy-active-before-data: Assert RDY signal one cycle before > data. > + If omitted it will be asserted with data. > + - nvidia,snor-rdy-inv: RDY signal is active high > + - nvidia,snor-adv-inv: ADV signal is active high > + - nvidia,snor-oe-inv: WE/OE signal is active high > + - nvidia,snor-cs-inv: CS signal is active high > + > + Note that there is some special handling for the timing values. > + From Tegra TRM: > + Programming 0 means 1 clock cycle: actual cycle = programmed cycle + 1 > + > + - nvidia,snor-muxed-width: Number of cycles MUX address/data asserted on the > + bus. Valid values are 0-15, default is 1 > + - nvidia,snor-hold-width: Number of cycles CE stays asserted after the > + de-assertion of WR_N (in case of SLAVE/MASTER Request) or OE_N > + (in case of MASTER Request). Valid values are 0-15, default is 1 > + - nvidia,snor-adv-width: Number of cycles during which ADV stays asserted. > + Valid values are 0-15, default is 1. > + - nvidia,snor-ce-width: Number of cycles before CE is asserted. > + Valid values are 0-15, default is 4 > + - nvidia,snor-we-width: Number of cycles during which WE stays asserted. > + Valid values are 0-15, default is 1 > + - nvidia,snor-oe-width: Number of cycles during which OE stays asserted. > + Valid values are 0-255, default is 1 > + - nvidia,snor-wait-width: Number of cycles before READY is asserted. > + Valid values are 0-255, default is 3 > + > +Example with two SJA1000 CAN controllers connected to the GMI bus. We wrap > the > +controllers with a simple-bus node since they are all connected to the sam
Re: [PATCH 5/6] i2c: tegra: Add runtime power-management support
On 25/08/16 20:26, Wolfram Sang wrote:
> * PGP Signed by an unknown key
>
>
>> @@ -407,32 +410,39 @@ static inline int tegra_i2c_clock_enable(struct
>> tegra_i2c_dev *i2c_dev)
>> return ret;
>> }
>> }
>> +
>> ret = clk_enable(i2c_dev->div_clk);
>> if (ret < 0) {
>> dev_err(i2c_dev->dev,
>> "Enabling div clk failed, err %d\n", ret);
>> clk_disable(i2c_dev->fast_clk);
>> +return ret;
>> }
>> -return ret;
>> +
>> +return 0;
>
> You could have left the original 'return' instead of the 2 new ones, but
> you decide.
Yes I know, but I wanted to ensure for runtime-pm we only return 0 on
success. Yes clk_enable should only return 0 on success and a negative
error code otherwise, but I prefer this. So will leave as-is.
>> -if (tegra_i2c_flush_fifos(i2c_dev))
>> -err = -ETIMEDOUT;
>> +err = tegra_i2c_flush_fifos(i2c_dev);
>
> 'err' is assigned but where is it checked?
It will be returned by the function. This is no different to how it
works today if you look at the code. I did think about checking it right
after this call and returning but then I am changing the behaviour and
that should be another patch. I am not sure why it is like this in the
first place, but I did not wish to introduce any different behaviour here.
Cheers
Jon
--
nvpublic
Re: [PATCH 2/6] i2c: tegra: Remove non device-tree support
On 25/08/16 20:33, Wolfram Sang wrote: > * PGP Signed by an unknown key > > On Thu, Aug 11, 2016 at 11:16:56AM +0100, Jon Hunter wrote: >> Tegra has only supported device-tree for platform/board configuration >> for quite some time now and so simplify the Tegra I2C driver by dropping >> code for non device-tree platforms/boards. >> >> Signed-off-by: Jon Hunter > > cppcheck rightfully says: > > drivers/i2c/busses/i2c-tegra.c:854: style: Variable 'i2c_dev.hw' is > reassigned a value before the old one has been used. > > Dropping the patches... Yes good catch. I will fix that. Cheers Jon -- nvpublic
Re: [PATCH v2 3/6] dt/bindings: Add bindings for Tegra GMI controller
On 26/08/16 05:53, Mirza Krak wrote:
...
>> I have an idea which is following:
>>
>> gmi@7009 {
>> status = "okay";
>> #address-cells = <2>;
>> #size-cells = <1>;
>> ranges = <4 0 0x4800 0x0004>;
>>
>> cs4 {
>> compatible = "simple-bus";
>> #address-cells = <2>;
>> #size-cells = <1>;
>> ranges;
>>
>> nvidia,snor-cs = <4>;
>> nvidia,snor-mux-mode;
>> nvidia,snor-adv-inv;
>>
>> can@0 {
>> compatible = "nxp,sja1000";
>> reg = <4 0 0x100>;
>> ...
>> };
>>
>>
>> can@4 {
>> compatible = "nxp,sja1000";
>> reg = <4 0x4 0x100>;
>> ...
>> };
>> };
>> };
>>
>> Do not know if above will work at all (not able to test at current
>> location), anyway I will play around with it some more and get back to
>> you.
>
> Gave above a test run and it works like a charm. Are we happy with that?
Does it not work with #address-cells = <1>? Seems odd to have the cs in
the reg for the device.
Cheers
Jon
--
nvpublic
Re: [PATCH v2 6/6] bus: Add support for Tegra Generic Memory Interface
t; + reset_control_assert(priv->rst);
> + udelay(2);
> + reset_control_deassert(priv->rst);
> +
> + tegra_gmi_parse_dt(dev, priv);
> + tegra_gmi_init(dev, priv);
> +
> + ret = of_platform_default_populate(dev->of_node, NULL, dev);
> + if (ret < 0) {
> + dev_err(dev, "fail to create devices.\n");
> + clk_disable_unprepare(priv->clk);
> + reset_control_assert(priv->rst);
nit ... I would assert the reset first and then disable the clock. This
allows the reset a few clocks to reset the logic.
Do you also need to clear the GO bit like in the remove here for
consistency? Could be worth adding a helper function to do this.
> + return ret;
> + }
> +
> + dev_set_drvdata(dev, priv);
> +
> + return 0;
> +}
> +
> +static int tegra_gmi_remove(struct platform_device *pdev)
> +{
> + struct tegra_gmi_priv *priv = dev_get_drvdata(&pdev->dev);
> + u32 config;
> +
> + of_platform_depopulate(&pdev->dev);
> +
> + config = readl(priv->base + TEGRA_GMI_CONFIG);
> + config &= ~TEGRA_GMI_CONFIG_GO;
> + writel(config, priv->base + TEGRA_GMI_CONFIG);
> +
> + clk_disable_unprepare(priv->clk);
> + reset_control_assert(priv->rst);
I would re-order the reset and clock here as well.
Otherwise ...
Reviewed-by: Jon Hunter
Cheers
Jon
--
nvpublic
[PATCH V2 1/9] i2c: tegra: Fix lines over 80 characters
Checkpatch warns about some lines over 80 characters in the Tegra I2C
driver and so fix these.
While we are at it, prefix the second instance of "STOP condition" in
the comment with a "the".
Signed-off-by: Jon Hunter
---
drivers/i2c/busses/i2c-tegra.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index d9979da11485..b90bc326907d 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -193,7 +193,8 @@ struct tegra_i2c_dev {
bool is_multimaster_mode;
};
-static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val, unsigned long
reg)
+static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val,
+ unsigned long reg)
{
writel(val, i2c_dev->base + reg);
}
@@ -643,9 +644,10 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev
*i2c_dev,
return 0;
/*
-* NACK interrupt is generated before the I2C controller generates the
-* STOP condition on the bus. So wait for 2 clock periods before
resetting
-* the controller so that STOP condition has been delivered properly.
+* NACK interrupt is generated before the I2C controller generates
+* the STOP condition on the bus. So wait for 2 clock periods
+* before resetting the controller so that the STOP condition has
+* been delivered properly.
*/
if (i2c_dev->msg_err == I2C_ERR_NO_ACK)
udelay(DIV_ROUND_UP(2 * 100, i2c_dev->bus_clk_rate));
--
2.1.4
[PATCH V2 0/9] Some Tegra I2C Updates
Add runtime-pm and pinctrl support for Tegra I2C driver. The first 6 patches are trivial clean-up/simplification changes. Changes since V1: - Fixed cppcheck warning reported by Wolfram - Added 3 more clean-up patches to fix some checkpatch issues Jon Hunter (9): i2c: tegra: Fix lines over 80 characters i2c: tegra: Use BIT macro i2c: tegra: Fix missing blank lines after declarations i2c: tegra: Add missing new line characters i2c: tegra: Remove non device-tree support i2c: tegra: Use device name for adapter name i2c: tegra: Simplify I2C resume i2c: tegra: Add runtime power-management support i2c: tegra: Add pinctrl support drivers/i2c/busses/i2c-tegra.c | 193 - 1 file changed, 114 insertions(+), 79 deletions(-) -- 2.1.4
[PATCH V2 3/9] i2c: tegra: Fix missing blank lines after declarations
Checkpatch warns about missing blank lines after declarations in the
Tegra I2C driver and so fix these.
Note that the initialisation of 'val' to zero in tegra_dvc_init() is
unnecessary and so remove this.
Signed-off-by: Jon Hunter
---
drivers/i2c/busses/i2c-tegra.c | 19 +--
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 98d13437eb42..7a7f899936a3 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -245,15 +245,17 @@ static void i2c_readsl(struct tegra_i2c_dev *i2c_dev,
void *data,
static void tegra_i2c_mask_irq(struct tegra_i2c_dev *i2c_dev, u32 mask)
{
- u32 int_mask = i2c_readl(i2c_dev, I2C_INT_MASK);
- int_mask &= ~mask;
+ u32 int_mask;
+
+ int_mask = i2c_readl(i2c_dev, I2C_INT_MASK) & ~mask;
i2c_writel(i2c_dev, int_mask, I2C_INT_MASK);
}
static void tegra_i2c_unmask_irq(struct tegra_i2c_dev *i2c_dev, u32 mask)
{
- u32 int_mask = i2c_readl(i2c_dev, I2C_INT_MASK);
- int_mask |= mask;
+ u32 int_mask;
+
+ int_mask = i2c_readl(i2c_dev, I2C_INT_MASK) | mask;
i2c_writel(i2c_dev, int_mask, I2C_INT_MASK);
}
@@ -261,6 +263,7 @@ static int tegra_i2c_flush_fifos(struct tegra_i2c_dev
*i2c_dev)
{
unsigned long timeout = jiffies + HZ;
u32 val = i2c_readl(i2c_dev, I2C_FIFO_CONTROL);
+
val |= I2C_FIFO_CONTROL_TX_FLUSH | I2C_FIFO_CONTROL_RX_FLUSH;
i2c_writel(i2c_dev, val, I2C_FIFO_CONTROL);
@@ -386,7 +389,8 @@ static int tegra_i2c_fill_tx_fifo(struct tegra_i2c_dev
*i2c_dev)
*/
static void tegra_dvc_init(struct tegra_i2c_dev *i2c_dev)
{
- u32 val = 0;
+ u32 val;
+
val = dvc_readl(i2c_dev, DVC_CTRL_REG3);
val |= DVC_CTRL_REG3_SW_PROG;
val |= DVC_CTRL_REG3_I2C_DONE_INTR_EN;
@@ -400,6 +404,7 @@ static void tegra_dvc_init(struct tegra_i2c_dev *i2c_dev)
static inline int tegra_i2c_clock_enable(struct tegra_i2c_dev *i2c_dev)
{
int ret;
+
if (!i2c_dev->hw->has_single_clk_source) {
ret = clk_enable(i2c_dev->fast_clk);
if (ret < 0) {
@@ -461,11 +466,11 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
if (!i2c_dev->is_dvc) {
u32 sl_cfg = i2c_readl(i2c_dev, I2C_SL_CNFG);
+
sl_cfg |= I2C_SL_CNFG_NACK | I2C_SL_CNFG_NEWSL;
i2c_writel(i2c_dev, sl_cfg, I2C_SL_CNFG);
i2c_writel(i2c_dev, 0xfc, I2C_SL_ADDR1);
i2c_writel(i2c_dev, 0x00, I2C_SL_ADDR2);
-
}
val = 7 << I2C_FIFO_CONTROL_TX_TRIG_SHIFT |
@@ -680,6 +685,7 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct
i2c_msg msgs[],
for (i = 0; i < num; i++) {
enum msg_end_type end_type = MSG_END_STOP;
+
if (i < (num - 1)) {
if (msgs[i + 1].flags & I2C_M_NOSTART)
end_type = MSG_END_CONTINUE;
@@ -956,6 +962,7 @@ unprepare_fast_clk:
static int tegra_i2c_remove(struct platform_device *pdev)
{
struct tegra_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
+
i2c_del_adapter(&i2c_dev->adapter);
if (i2c_dev->is_multimaster_mode)
--
2.1.4
[PATCH V2 2/9] i2c: tegra: Use BIT macro
Checkpatch warns about spacing around the '<<' operator in the Tegra I2C driver and so fix these by converting the bit definitions that are using this operator to use the BIT macro. Signed-off-by: Jon Hunter --- drivers/i2c/busses/i2c-tegra.c | 66 +- 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c index b90bc326907d..98d13437eb42 100644 --- a/drivers/i2c/busses/i2c-tegra.c +++ b/drivers/i2c/busses/i2c-tegra.c @@ -36,21 +36,21 @@ #define I2C_CNFG 0x000 #define I2C_CNFG_DEBOUNCE_CNT_SHIFT12 -#define I2C_CNFG_PACKET_MODE_EN(1<<10) -#define I2C_CNFG_NEW_MASTER_FSM(1<<11) -#define I2C_CNFG_MULTI_MASTER_MODE (1<<17) +#define I2C_CNFG_PACKET_MODE_ENBIT(10) +#define I2C_CNFG_NEW_MASTER_FSMBIT(11) +#define I2C_CNFG_MULTI_MASTER_MODE BIT(17) #define I2C_STATUS 0x01C #define I2C_SL_CNFG0x020 -#define I2C_SL_CNFG_NACK (1<<1) -#define I2C_SL_CNFG_NEWSL (1<<2) +#define I2C_SL_CNFG_NACK BIT(1) +#define I2C_SL_CNFG_NEWSL BIT(2) #define I2C_SL_ADDR1 0x02c #define I2C_SL_ADDR2 0x030 #define I2C_TX_FIFO0x050 #define I2C_RX_FIFO0x054 #define I2C_PACKET_TRANSFER_STATUS 0x058 #define I2C_FIFO_CONTROL 0x05c -#define I2C_FIFO_CONTROL_TX_FLUSH (1<<1) -#define I2C_FIFO_CONTROL_RX_FLUSH (1<<0) +#define I2C_FIFO_CONTROL_TX_FLUSH BIT(1) +#define I2C_FIFO_CONTROL_RX_FLUSH BIT(0) #define I2C_FIFO_CONTROL_TX_TRIG_SHIFT 5 #define I2C_FIFO_CONTROL_RX_TRIG_SHIFT 2 #define I2C_FIFO_STATUS0x060 @@ -60,26 +60,26 @@ #define I2C_FIFO_STATUS_RX_SHIFT 0 #define I2C_INT_MASK 0x064 #define I2C_INT_STATUS 0x068 -#define I2C_INT_PACKET_XFER_COMPLETE (1<<7) -#define I2C_INT_ALL_PACKETS_XFER_COMPLETE (1<<6) -#define I2C_INT_TX_FIFO_OVERFLOW (1<<5) -#define I2C_INT_RX_FIFO_UNDERFLOW (1<<4) -#define I2C_INT_NO_ACK (1<<3) -#define I2C_INT_ARBITRATION_LOST (1<<2) -#define I2C_INT_TX_FIFO_DATA_REQ (1<<1) -#define I2C_INT_RX_FIFO_DATA_REQ (1<<0) +#define I2C_INT_PACKET_XFER_COMPLETE BIT(7) +#define I2C_INT_ALL_PACKETS_XFER_COMPLETE BIT(6) +#define I2C_INT_TX_FIFO_OVERFLOW BIT(5) +#define I2C_INT_RX_FIFO_UNDERFLOW BIT(4) +#define I2C_INT_NO_ACK BIT(3) +#define I2C_INT_ARBITRATION_LOST BIT(2) +#define I2C_INT_TX_FIFO_DATA_REQ BIT(1) +#define I2C_INT_RX_FIFO_DATA_REQ BIT(0) #define I2C_CLK_DIVISOR0x06c #define I2C_CLK_DIVISOR_STD_FAST_MODE_SHIFT16 #define I2C_CLK_MULTIPLIER_STD_FAST_MODE 8 #define DVC_CTRL_REG1 0x000 -#define DVC_CTRL_REG1_INTR_EN (1<<10) +#define DVC_CTRL_REG1_INTR_EN BIT(10) #define DVC_CTRL_REG2 0x004 #define DVC_CTRL_REG3 0x008 -#define DVC_CTRL_REG3_SW_PROG (1<<26) -#define DVC_CTRL_REG3_I2C_DONE_INTR_EN (1<<30) +#define DVC_CTRL_REG3_SW_PROG BIT(26) +#define DVC_CTRL_REG3_I2C_DONE_INTR_EN BIT(30) #define DVC_STATUS 0x00c -#define DVC_STATUS_I2C_DONE_INTR (1<<30) +#define DVC_STATUS_I2C_DONE_INTR BIT(30) #define I2C_ERR_NONE 0x00 #define I2C_ERR_NO_ACK 0x01 @@ -89,26 +89,26 @@ #define PACKET_HEADER0_HEADER_SIZE_SHIFT 28 #define PACKET_HEADER0_PACKET_ID_SHIFT 16 #define PACKET_HEADER0_CONT_ID_SHIFT 12 -#define PACKET_HEADER0_PROTOCOL_I2C(1<<4) - -#define I2C_HEADER_HIGHSPEED_MODE (1<<22) -#define I2C_HEADER_CONT_ON_NAK (1<<21) -#define I2C_HEADER_SEND_START_BYTE (1<<20) -#define I2C_HEADER_READ(1<<19) -#define I2C_HEADER_10BIT_ADDR (1<<18) -#define I2C_HEADER_IE_ENABLE (1<<17) -#define I2C_HEADER_REPEAT_START(1<<16) -#define I2C_HEADER_CONTINUE_XFER (1<<15) +#define PACKET_HEADER0_PROTOCOL_I2CB
[PATCH V2 5/9] i2c: tegra: Remove non device-tree support
Tegra has only supported device-tree for platform/board configuration
for quite some time now and so simplify the Tegra I2C driver by dropping
code for non device-tree platforms/boards.
Signed-off-by: Jon Hunter
---
drivers/i2c/busses/i2c-tegra.c | 12 +++-
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 7f31a103c04a..e5c8c67a2491 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -857,15 +857,9 @@ static int tegra_i2c_probe(struct platform_device *pdev)
tegra_i2c_parse_dt(i2c_dev);
- i2c_dev->hw = &tegra20_i2c_hw;
-
- if (pdev->dev.of_node) {
- i2c_dev->hw = of_device_get_match_data(&pdev->dev);
- i2c_dev->is_dvc = of_device_is_compatible(pdev->dev.of_node,
- "nvidia,tegra20-i2c-dvc");
- } else if (pdev->id == 3) {
- i2c_dev->is_dvc = 1;
- }
+ i2c_dev->hw = of_device_get_match_data(&pdev->dev);
+ i2c_dev->is_dvc = of_device_is_compatible(pdev->dev.of_node,
+ "nvidia,tegra20-i2c-dvc");
init_completion(&i2c_dev->msg_complete);
if (!i2c_dev->hw->has_single_clk_source) {
--
2.1.4
[PATCH V2 6/9] i2c: tegra: Use device name for adapter name
All Tegra I2C devices have the name "Tegra I2C adapter" which is not very useful when viewing the I2C adapter names via the sysfs. Therefore, use the device name, which is unique for each I2C device, instead. Signed-off-by: Jon Hunter Acked-by: Laxman Dewangan --- drivers/i2c/busses/i2c-tegra.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c index e5c8c67a2491..bdb50258a9a8 100644 --- a/drivers/i2c/busses/i2c-tegra.c +++ b/drivers/i2c/busses/i2c-tegra.c @@ -927,7 +927,7 @@ static int tegra_i2c_probe(struct platform_device *pdev) i2c_set_adapdata(&i2c_dev->adapter, i2c_dev); i2c_dev->adapter.owner = THIS_MODULE; i2c_dev->adapter.class = I2C_CLASS_DEPRECATED; - strlcpy(i2c_dev->adapter.name, "Tegra I2C adapter", + strlcpy(i2c_dev->adapter.name, dev_name(&pdev->dev), sizeof(i2c_dev->adapter.name)); i2c_dev->adapter.dev.parent = &pdev->dev; i2c_dev->adapter.nr = pdev->id; -- 2.1.4
[PATCH V2 8/9] i2c: tegra: Add runtime power-management support
Update the Tegra I2C driver to use runtime PM and move the code in the
tegra_i2c_clock_enable/disable() functions to the PM runtime resume and
suspend callbacks, respectively.
Note that given that CONFIG_PM is not mandatory for Tegra, if CONFIG_PM
is not enabled and so runtime PM is not enabled, ensure that the I2C
clocks are turned on during probe and kept on by calling the resume
callback directly.
In the function tegra_i2c_init(), the variable 'err' does not need to be
initialised to zero in tegra_i2c_init() because it is initialised when
pm_runtime_get_sync() is called. Furthermore, to ensure we only return 0
from tegra_i2c_init(), it is necessary to re-initialise 'err' to 0 after
a successful call to pm_runtime_get_sync() because it can return a
positive value on success. However, alternatively re-initialise 'err' by
using the return value of the function tegra_i2c_flush_fifos() because
it can only be 0 or -ETIMEDOUT.
Signed-off-by: Jon Hunter
Acked-by: Laxman Dewangan
---
drivers/i2c/busses/i2c-tegra.c | 60 --
1 file changed, 46 insertions(+), 14 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 3c27012fa96c..05e34dc29d5a 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -28,6 +28,7 @@
#include
#include
#include
+#include
#include
@@ -401,8 +402,9 @@ static void tegra_dvc_init(struct tegra_i2c_dev *i2c_dev)
dvc_writel(i2c_dev, val, DVC_CTRL_REG1);
}
-static inline int tegra_i2c_clock_enable(struct tegra_i2c_dev *i2c_dev)
+static int tegra_i2c_runtime_resume(struct device *dev)
{
+ struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev);
int ret;
if (!i2c_dev->hw->has_single_clk_source) {
@@ -413,32 +415,39 @@ static inline int tegra_i2c_clock_enable(struct
tegra_i2c_dev *i2c_dev)
return ret;
}
}
+
ret = clk_enable(i2c_dev->div_clk);
if (ret < 0) {
dev_err(i2c_dev->dev,
"Enabling div clk failed, err %d\n", ret);
clk_disable(i2c_dev->fast_clk);
+ return ret;
}
- return ret;
+
+ return 0;
}
-static inline void tegra_i2c_clock_disable(struct tegra_i2c_dev *i2c_dev)
+static int tegra_i2c_runtime_suspend(struct device *dev)
{
+ struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev);
+
clk_disable(i2c_dev->div_clk);
if (!i2c_dev->hw->has_single_clk_source)
clk_disable(i2c_dev->fast_clk);
+
+ return 0;
}
static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
{
u32 val;
- int err = 0;
+ int err;
u32 clk_divisor;
unsigned long timeout = jiffies + HZ;
- err = tegra_i2c_clock_enable(i2c_dev);
+ err = pm_runtime_get_sync(i2c_dev->dev);
if (err < 0) {
- dev_err(i2c_dev->dev, "Clock enable failed %d\n", err);
+ dev_err(i2c_dev->dev, "runtime resume failed %d\n", err);
return err;
}
@@ -477,8 +486,7 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
0 << I2C_FIFO_CONTROL_RX_TRIG_SHIFT;
i2c_writel(i2c_dev, val, I2C_FIFO_CONTROL);
- if (tegra_i2c_flush_fifos(i2c_dev))
- err = -ETIMEDOUT;
+ err = tegra_i2c_flush_fifos(i2c_dev);
if (i2c_dev->is_multimaster_mode && i2c_dev->hw->has_slcg_override_reg)
i2c_writel(i2c_dev, I2C_MST_CORE_CLKEN_OVR, I2C_CLKEN_OVERRIDE);
@@ -502,7 +510,7 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
}
err:
- tegra_i2c_clock_disable(i2c_dev);
+ pm_runtime_put(i2c_dev->dev);
return err;
}
@@ -677,9 +685,9 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct
i2c_msg msgs[],
if (i2c_dev->is_suspended)
return -EBUSY;
- ret = tegra_i2c_clock_enable(i2c_dev);
+ ret = pm_runtime_get_sync(i2c_dev->dev);
if (ret < 0) {
- dev_err(i2c_dev->dev, "Clock enable failed %d\n", ret);
+ dev_err(i2c_dev->dev, "runtime resume failed %d\n", ret);
return ret;
}
@@ -696,7 +704,9 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct
i2c_msg msgs[],
if (ret)
break;
}
- tegra_i2c_clock_disable(i2c_dev);
+
+ pm_runtime_put(i2c_dev->dev);
+
return ret ?: i;
}
@@ -902,12 +912,21 @@ static int tegra_i2c_probe(struct platform_device *pdev)
goto unprepare_fast_clk;
}
+ pm_runtime_enable(&pdev->dev);
+ if (!pm_runtime_enabled(&pdev->dev)) {
+ ret = tegra_i
[PATCH V2 7/9] i2c: tegra: Simplify I2C resume
The I2C adapter is unlocked regardless of whether the tegra_i2c_init()
called during the resume is successful or not. However, if the
tegra_i2c_init() is not successful, then ->is_suspended is not set to
false. Simplify the resume code by only setting ->is_suspended to false
if tegra_i2c_init() is successful and return the error code from
tegra_i2c_init().
Signed-off-by: Jon Hunter
Acked-by: Laxman Dewangan
---
drivers/i2c/busses/i2c-tegra.c | 11 +++
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index bdb50258a9a8..3c27012fa96c 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -989,17 +989,12 @@ static int tegra_i2c_resume(struct device *dev)
i2c_lock_adapter(&i2c_dev->adapter);
ret = tegra_i2c_init(i2c_dev);
-
- if (ret) {
- i2c_unlock_adapter(&i2c_dev->adapter);
- return ret;
- }
-
- i2c_dev->is_suspended = false;
+ if (!ret)
+ i2c_dev->is_suspended = false;
i2c_unlock_adapter(&i2c_dev->adapter);
- return 0;
+ return ret;
}
static SIMPLE_DEV_PM_OPS(tegra_i2c_pm, tegra_i2c_suspend, tegra_i2c_resume);
--
2.1.4
[PATCH V2 9/9] i2c: tegra: Add pinctrl support
On Tegra124/132 the pins for I2C6 are shared with the Display Port AUX
(DPAUX) channel and on Tegra210 the pins for I2C4 and I2C6 are shared
with DPAUX1 and DPAUX0, respectively. The multiplexing of the pins is
handled by a register in the DPAUX and so the Tegra DPAUX driver has
been updated to register a pinctrl device for managing these pins.
The pins for these particular I2C devices are bound to the I2C device
prior to probing. However, these I2C devices are in a different power
partition to the DPAUX devices that own the pins. Hence, it is desirable
to place the pins in the 'idle' state and allow the DPAUX power
partition to switch off, when these I2C devices is not in use.
Therefore, add calls to place the I2C pins in the 'default' and 'idle'
states when the I2C device is runtime resumed and suspended,
respectively.
Please note that the pinctrl functions that set the state of the pins
check to see if the devices has pins associated and will return zero
if they do not. Therefore, it is safe to call these pinctrl functions
even for I2C devices that do not have any pins associated.
Signed-off-by: Jon Hunter
Acked-by: Laxman Dewangan
---
drivers/i2c/busses/i2c-tegra.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 05e34dc29d5a..d86a993b75d6 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -28,6 +28,7 @@
#include
#include
#include
+#include
#include
#include
@@ -407,6 +408,10 @@ static int tegra_i2c_runtime_resume(struct device *dev)
struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev);
int ret;
+ ret = pinctrl_pm_select_default_state(i2c_dev->dev);
+ if (ret)
+ return ret;
+
if (!i2c_dev->hw->has_single_clk_source) {
ret = clk_enable(i2c_dev->fast_clk);
if (ret < 0) {
@@ -435,7 +440,7 @@ static int tegra_i2c_runtime_suspend(struct device *dev)
if (!i2c_dev->hw->has_single_clk_source)
clk_disable(i2c_dev->fast_clk);
- return 0;
+ return pinctrl_pm_select_idle_state(i2c_dev->dev);
}
static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
--
2.1.4
[PATCH V2 4/9] i2c: tegra: Add missing new line characters
Add missing new line characters for the various error messages.
Signed-off-by: Jon Hunter
Acked-by: Laxman Dewangan
---
drivers/i2c/busses/i2c-tegra.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 7a7f899936a3..7f31a103c04a 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -833,7 +833,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
div_clk = devm_clk_get(&pdev->dev, "div-clk");
if (IS_ERR(div_clk)) {
- dev_err(&pdev->dev, "missing controller clock");
+ dev_err(&pdev->dev, "missing controller clock\n");
return PTR_ERR(div_clk);
}
@@ -851,7 +851,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
i2c_dev->rst = devm_reset_control_get(&pdev->dev, "i2c");
if (IS_ERR(i2c_dev->rst)) {
- dev_err(&pdev->dev, "missing controller reset");
+ dev_err(&pdev->dev, "missing controller reset\n");
return PTR_ERR(i2c_dev->rst);
}
@@ -871,7 +871,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
if (!i2c_dev->hw->has_single_clk_source) {
fast_clk = devm_clk_get(&pdev->dev, "fast-clk");
if (IS_ERR(fast_clk)) {
- dev_err(&pdev->dev, "missing fast clock");
+ dev_err(&pdev->dev, "missing fast clock\n");
return PTR_ERR(fast_clk);
}
i2c_dev->fast_clk = fast_clk;
@@ -919,7 +919,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
ret = tegra_i2c_init(i2c_dev);
if (ret) {
- dev_err(&pdev->dev, "Failed to initialize i2c controller");
+ dev_err(&pdev->dev, "Failed to initialize i2c controller\n");
goto disable_div_clk;
}
--
2.1.4
Re: [PATCH V2 0/9] Some Tegra I2C Updates
On 26/08/16 16:56, Wolfram Sang wrote: > * PGP Signed by an unknown key > > On Fri, Aug 26, 2016 at 02:08:56PM +0100, Jon Hunter wrote: >> Add runtime-pm and pinctrl support for Tegra I2C driver. >> >> The first 6 patches are trivial clean-up/simplification changes. >> >> Changes since V1: >> - Fixed cppcheck warning reported by Wolfram >> - Added 3 more clean-up patches to fix some checkpatch issues > > Thanks looks good. Now we only need to find out which one to apply > first, this one or "[v8,1/3] i2c: tegra: use readx_poll_timeout after > config_load reg programmed" and following. I cced you on the other mail. Yes I had not bothered basing mine on top as it seem to be stagnant. However, if Shardar updates his series I am happy to rebase. I will be out for a week and so I will pick this up when I get back. Cheers! Jon -- nvpublic
Re: [PATCH V2 9/9] i2c: tegra: Add pinctrl support
On 26/08/16 16:55, Stephen Warren wrote: > On 08/26/2016 07:09 AM, Jon Hunter wrote: >> On Tegra124/132 the pins for I2C6 are shared with the Display Port AUX >> (DPAUX) channel and on Tegra210 the pins for I2C4 and I2C6 are shared >> with DPAUX1 and DPAUX0, respectively. The multiplexing of the pins is >> handled by a register in the DPAUX and so the Tegra DPAUX driver has >> been updated to register a pinctrl device for managing these pins. >> >> The pins for these particular I2C devices are bound to the I2C device >> prior to probing. However, these I2C devices are in a different power >> partition to the DPAUX devices that own the pins. Hence, it is desirable >> to place the pins in the 'idle' state and allow the DPAUX power >> partition to switch off, when these I2C devices is not in use. >> Therefore, add calls to place the I2C pins in the 'default' and 'idle' >> states when the I2C device is runtime resumed and suspended, >> respectively. >> >> Please note that the pinctrl functions that set the state of the pins >> check to see if the devices has pins associated and will return zero >> if they do not. Therefore, it is safe to call these pinctrl functions >> even for I2C devices that do not have any pins associated. > > I think this should be handled by drivers/i2c/muxes/i2c-mux-pinctrl.c > instead? I remember having a look at i2c-mux some time back, but we did not have requirement to share the pins dynamically at runtime between the DPAUX and I2C devices. The pins are just configured at probe time for either the DPAUX or I2C device and then with this change when we are not active we can power down the pins. However, the pins are always bound to either the DPAUX or I2C. Cheers Jon -- nvpublic
Re: [PATCH v5 00/13] gpio: Tight IRQ chip integration
Hi Linus, On 13/10/17 16:49, Thierry Reding wrote: > From: Thierry Reding > > Hi Linus, > > here's the latest series of patches that implement the tighter IRQ chip > integration. I've dropped the banked infrastructure for now as per the > discussion with Grygorii. > > The first couple of patches are mostly preparatory work in order to > consolidate all IRQ chip related fields in a new structure and create > the base functionality for adding IRQ chips. > > After that, I've added the Tegra186 GPIO support patch that makes use of > the new tight integration. I have reviewed this series and tested on Tegra, so for the series ... Reviewed-by: Jon Hunter Tested-by: Jon Hunter We would really like to get support for Tegra186 GPIO in v4.15, so please let us know if you think that this is do-able. Cheers Jon -- nvpublic
Re: [PATCH v5 00/13] gpio: Tight IRQ chip integration
Ping! On 23/10/17 09:47, Jon Hunter wrote: > Hi Linus, > > On 16/10/17 19:09, Jon Hunter wrote: >> Hi Linus, >> >> On 13/10/17 16:49, Thierry Reding wrote: >>> From: Thierry Reding >>> >>> Hi Linus, >>> >>> here's the latest series of patches that implement the tighter IRQ chip >>> integration. I've dropped the banked infrastructure for now as per the >>> discussion with Grygorii. >>> >>> The first couple of patches are mostly preparatory work in order to >>> consolidate all IRQ chip related fields in a new structure and create >>> the base functionality for adding IRQ chips. >>> >>> After that, I've added the Tegra186 GPIO support patch that makes use of >>> the new tight integration. >> >> I have reviewed this series and tested on Tegra, so for the series ... >> >> Reviewed-by: Jon Hunter >> Tested-by: Jon Hunter >> >> We would really like to get support for Tegra186 GPIO in v4.15, so >> please let us know if you think that this is do-able. > > Any feedback/comments? > > Thanks > Jon > -- nvpublic
[PATCH 2/2] mfd: cros ec: spi: Simplify delay handling between SPI messages
The EC SPI driver prevents SPI transfers being to rapidly by keeping
track of the time the last transfer was issued via the
'last_transfer_ns' variable. Previously, if the 'last_transfer_ns'
variable was zero, this indicated that no previous transfer had been
sent and that no delay was needed. However, the EC SPI driver has
been updated to always initialise the 'last_transfer_ns' variable
during probe and therefore, it is no longer necessary to test if it
is zero. Remove the code that checks if this variable is zero.
Signed-off-by: Jon Hunter
---
drivers/mfd/cros_ec_spi.c | 25 +
1 file changed, 9 insertions(+), 16 deletions(-)
diff --git a/drivers/mfd/cros_ec_spi.c b/drivers/mfd/cros_ec_spi.c
index a14196e95e9b..7849d781c6bb 100644
--- a/drivers/mfd/cros_ec_spi.c
+++ b/drivers/mfd/cros_ec_spi.c
@@ -72,8 +72,7 @@
* struct cros_ec_spi - information about a SPI-connected EC
*
* @spi: SPI device we are connected to
- * @last_transfer_ns: time that we last finished a transfer, or 0 if there
- * if no record
+ * @last_transfer_ns: time that we last finished a transfer.
* @start_of_msg_delay: used to set the delay_usecs on the spi_transfer that
* is sent when we want to turn on CS at the start of a transaction.
* @end_of_msg_delay: used to set the delay_usecs on the spi_transfer that
@@ -378,18 +377,15 @@ static int cros_ec_pkt_xfer_spi(struct cros_ec_device
*ec_dev,
u8 *rx_buf;
u8 sum;
int ret = 0, final_ret;
+ unsigned long delay;
len = cros_ec_prepare_tx(ec_dev, ec_msg);
dev_dbg(ec_dev->dev, "prepared, len=%d\n", len);
/* If it's too soon to do another transaction, wait */
- if (ec_spi->last_transfer_ns) {
- unsigned long delay;/* The delay completed so far */
-
- delay = ktime_get_ns() - ec_spi->last_transfer_ns;
- if (delay < EC_SPI_RECOVERY_TIME_NS)
- ndelay(EC_SPI_RECOVERY_TIME_NS - delay);
- }
+ delay = ktime_get_ns() - ec_spi->last_transfer_ns;
+ if (delay < EC_SPI_RECOVERY_TIME_NS)
+ ndelay(EC_SPI_RECOVERY_TIME_NS - delay);
rx_buf = kzalloc(len, GFP_KERNEL);
if (!rx_buf)
@@ -510,18 +506,15 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device
*ec_dev,
u8 *rx_buf;
int sum;
int ret = 0, final_ret;
+ unsigned long delay;
len = cros_ec_prepare_tx(ec_dev, ec_msg);
dev_dbg(ec_dev->dev, "prepared, len=%d\n", len);
/* If it's too soon to do another transaction, wait */
- if (ec_spi->last_transfer_ns) {
- unsigned long delay;/* The delay completed so far */
-
- delay = ktime_get_ns() - ec_spi->last_transfer_ns;
- if (delay < EC_SPI_RECOVERY_TIME_NS)
- ndelay(EC_SPI_RECOVERY_TIME_NS - delay);
- }
+ delay = ktime_get_ns() - ec_spi->last_transfer_ns;
+ if (delay < EC_SPI_RECOVERY_TIME_NS)
+ ndelay(EC_SPI_RECOVERY_TIME_NS - delay);
rx_buf = kzalloc(len, GFP_KERNEL);
if (!rx_buf)
--
2.7.4
[PATCH 1/2] mfd: cros ec: spi: Don't send first message too soon
On the Tegra124 Nyan-Big chromebook the very first SPI message sent to
the EC is failing.
The Tegra SPI driver configures the SPI chip-selects to be active-high
by default (and always has for many years). The EC SPI requires an
active-low chip-select and so the Tegra chip-select is reconfigured to
be active-low when the EC SPI driver calls spi_setup(). The problem is
that if the first SPI message to the EC is sent too soon after
reconfiguring the SPI chip-select, it fails.
The EC SPI driver prevents back-to-back SPI messages being sent too
soon by keeping track of the time the last transfer was sent via the
variable 'last_transfer_ns'. To prevent the very first transfer being
sent too soon, initialise the 'last_transfer_ns' variable after calling
spi_setup() and before sending the first SPI message.
Signed-off-by: Jon Hunter
---
Looks like this issue has been around for several Linux releases now
and it just depends on timing if this issue is seen or not and so there
is no specific commit this fixes. However, would be good to include for
v4.15.
drivers/mfd/cros_ec_spi.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/mfd/cros_ec_spi.c b/drivers/mfd/cros_ec_spi.c
index c9714072e224..a14196e95e9b 100644
--- a/drivers/mfd/cros_ec_spi.c
+++ b/drivers/mfd/cros_ec_spi.c
@@ -667,6 +667,7 @@ static int cros_ec_spi_probe(struct spi_device *spi)
sizeof(struct ec_response_get_protocol_info);
ec_dev->dout_size = sizeof(struct ec_host_request);
+ ec_spi->last_transfer_ns = ktime_get_ns();
err = cros_ec_register(ec_dev);
if (err) {
--
2.7.4
[PATCH V2 1/2] mfd: cros ec: spi: Don't send first message too soon
On the Tegra124 Nyan-Big chromebook the very first SPI message sent to
the EC is failing.
The Tegra SPI driver configures the SPI chip-selects to be active-high
by default (and always has for many years). The EC SPI requires an
active-low chip-select and so the Tegra chip-select is reconfigured to
be active-low when the EC SPI driver calls spi_setup(). The problem is
that if the first SPI message to the EC is sent too soon after
reconfiguring the SPI chip-select, it fails.
The EC SPI driver prevents back-to-back SPI messages being sent too
soon by keeping track of the time the last transfer was sent via the
variable 'last_transfer_ns'. To prevent the very first transfer being
sent too soon, initialise the 'last_transfer_ns' variable after calling
spi_setup() and before sending the first SPI message.
Cc:
Signed-off-by: Jon Hunter
Reviewed-by: Brian Norris
---
Changes since V1:
- Added stable-tag and Brian's reviewed-by.
Looks like this issue has been around for several Linux releases now
and it just depends on timing if this issue is seen or not and so there
is no specific commit this fixes. However, would be good to include for
v4.15.
drivers/mfd/cros_ec_spi.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/mfd/cros_ec_spi.c b/drivers/mfd/cros_ec_spi.c
index c9714072e224..a14196e95e9b 100644
--- a/drivers/mfd/cros_ec_spi.c
+++ b/drivers/mfd/cros_ec_spi.c
@@ -667,6 +667,7 @@ static int cros_ec_spi_probe(struct spi_device *spi)
sizeof(struct ec_response_get_protocol_info);
ec_dev->dout_size = sizeof(struct ec_host_request);
+ ec_spi->last_transfer_ns = ktime_get_ns();
err = cros_ec_register(ec_dev);
if (err) {
--
2.7.4
[PATCH V2 2/2] mfd: cros ec: spi: Simplify delay handling between SPI messages
The EC SPI driver prevents SPI transfers being to rapidly by keeping
track of the time the last transfer was issued via the
'last_transfer_ns' variable. Previously, if the 'last_transfer_ns'
variable was zero, this indicated that no previous transfer had been
sent and that no delay was needed. However, the EC SPI driver has
been updated to always initialise the 'last_transfer_ns' variable
during probe and therefore, it is no longer necessary to test if it
is zero. Remove the code that checks if this variable is zero.
Signed-off-by: Jon Hunter
Reviewed-by: Brian Norris
---
Changes since V1:
- Added Brian's reviewed-by.
drivers/mfd/cros_ec_spi.c | 25 +
1 file changed, 9 insertions(+), 16 deletions(-)
diff --git a/drivers/mfd/cros_ec_spi.c b/drivers/mfd/cros_ec_spi.c
index a14196e95e9b..7849d781c6bb 100644
--- a/drivers/mfd/cros_ec_spi.c
+++ b/drivers/mfd/cros_ec_spi.c
@@ -72,8 +72,7 @@
* struct cros_ec_spi - information about a SPI-connected EC
*
* @spi: SPI device we are connected to
- * @last_transfer_ns: time that we last finished a transfer, or 0 if there
- * if no record
+ * @last_transfer_ns: time that we last finished a transfer.
* @start_of_msg_delay: used to set the delay_usecs on the spi_transfer that
* is sent when we want to turn on CS at the start of a transaction.
* @end_of_msg_delay: used to set the delay_usecs on the spi_transfer that
@@ -378,18 +377,15 @@ static int cros_ec_pkt_xfer_spi(struct cros_ec_device
*ec_dev,
u8 *rx_buf;
u8 sum;
int ret = 0, final_ret;
+ unsigned long delay;
len = cros_ec_prepare_tx(ec_dev, ec_msg);
dev_dbg(ec_dev->dev, "prepared, len=%d\n", len);
/* If it's too soon to do another transaction, wait */
- if (ec_spi->last_transfer_ns) {
- unsigned long delay;/* The delay completed so far */
-
- delay = ktime_get_ns() - ec_spi->last_transfer_ns;
- if (delay < EC_SPI_RECOVERY_TIME_NS)
- ndelay(EC_SPI_RECOVERY_TIME_NS - delay);
- }
+ delay = ktime_get_ns() - ec_spi->last_transfer_ns;
+ if (delay < EC_SPI_RECOVERY_TIME_NS)
+ ndelay(EC_SPI_RECOVERY_TIME_NS - delay);
rx_buf = kzalloc(len, GFP_KERNEL);
if (!rx_buf)
@@ -510,18 +506,15 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device
*ec_dev,
u8 *rx_buf;
int sum;
int ret = 0, final_ret;
+ unsigned long delay;
len = cros_ec_prepare_tx(ec_dev, ec_msg);
dev_dbg(ec_dev->dev, "prepared, len=%d\n", len);
/* If it's too soon to do another transaction, wait */
- if (ec_spi->last_transfer_ns) {
- unsigned long delay;/* The delay completed so far */
-
- delay = ktime_get_ns() - ec_spi->last_transfer_ns;
- if (delay < EC_SPI_RECOVERY_TIME_NS)
- ndelay(EC_SPI_RECOVERY_TIME_NS - delay);
- }
+ delay = ktime_get_ns() - ec_spi->last_transfer_ns;
+ if (delay < EC_SPI_RECOVERY_TIME_NS)
+ ndelay(EC_SPI_RECOVERY_TIME_NS - delay);
rx_buf = kzalloc(len, GFP_KERNEL);
if (!rx_buf)
--
2.7.4
