Devicetree
 help / color / mirror / Atom feed
* Re: [PATCH v4] mfd: syscon: Add hardware spinlock support
From: Baolin Wang @ 2017-11-30  8:50 UTC (permalink / raw)
  To: Lee Jones
  Cc: Arnd Bergmann, Rob Herring, Mark Rutland, Mark Brown, LKML, DTML
In-Reply-To: <20171130083552.ynti3n3njzvps555@dell>

On 30 November 2017 at 16:35, Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Thu, 30 Nov 2017, Baolin Wang wrote:
>
>> Some system control registers need hardware spinlock to synchronize
>> between the multiple subsystems, so we should add hardware spinlock
>> support for syscon.
>>
>> Signed-off-by: Baolin Wang <baolin.wang-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> ---
>> Changes since v3:
>>  - Add error handling for of_hwspin_lock_get_id()
>>
>> Changes since v2:
>>  - Add acked tag from Rob.
>>
>> Changes since v1:
>>  - Remove timeout configuration.
>>  - Modify the binding file to add hwlocks.
>> ---
>>  Documentation/devicetree/bindings/mfd/syscon.txt |    1 +
>>  drivers/mfd/syscon.c                             |   21 +++++++++++++++++++++
>>  2 files changed, 22 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/syscon.txt b/Documentation/devicetree/bindings/mfd/syscon.txt
>> index 8b92d45..f464cc8 100644
>> --- a/Documentation/devicetree/bindings/mfd/syscon.txt
>> +++ b/Documentation/devicetree/bindings/mfd/syscon.txt
>> @@ -16,6 +16,7 @@ Required properties:
>>  Optional property:
>>  - reg-io-width: the size (in bytes) of the IO accesses that should be
>>    performed on the device.
>> +- hwlocks: reference to a phandle of a hardware spinlock provider node.
>
> Example?
>
> Perhaps even include the hwlock node (I'm not sure what that would
> look like).

Sure, I will add one example node here.

>
>>  Examples:
>>  gpr: iomuxc-gpr@20e0000 {
>> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
>> index b93fe4c..caf6a25 100644
>> --- a/drivers/mfd/syscon.c
>> +++ b/drivers/mfd/syscon.c
>> @@ -13,6 +13,7 @@
>>   */
>>
>>  #include <linux/err.h>
>> +#include <linux/hwspinlock.h>
>>  #include <linux/io.h>
>>  #include <linux/module.h>
>>  #include <linux/list.h>
>> @@ -87,6 +88,26 @@ static struct syscon *of_syscon_register(struct device_node *np)
>>       if (ret)
>>               reg_io_width = 4;
>>
>> +     ret = of_hwspin_lock_get_id(np, 0);
>> +     if (ret > 0) {
>> +             syscon_config.hwlock_id = ret;
>> +             syscon_config.hwlock_mode = HWLOCK_IRQSTATE;
>> +     } else {
>> +             switch (ret) {
>> +             case -ENOENT:
>> +                     /* ignore missing hwlock, it's optional */
>
> Is your shift key broken? ;)
>
> Please use proper grammar.  Sentences start with capital letters.

OK. Thanks for your comments.

-- 
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* [PATCH v5] mfd: syscon: Add hardware spinlock support
From: Baolin Wang @ 2017-11-30  9:10 UTC (permalink / raw)
  To: lee.jones-QSEj5FYQhm4dnm+yROfE0A, arnd-r2nGTMty4D4,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8
  Cc: broonie-DgEjT+Ai2ygdnm+yROfE0A,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	baolin.wang-QSEj5FYQhm4dnm+yROfE0A

Some system control registers need hardware spinlock to synchronize
between the multiple subsystems, so we should add hardware spinlock
support for syscon.

Signed-off-by: Baolin Wang <baolin.wang-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
Changes since v4:
 - Add one exapmle to show how to add hwlock.
 - Fix the coding style issue.

Changes since v3:
 - Add error handling for of_hwspin_lock_get_id()

Changes since v2:
 - Add acked tag from Rob.

Changes since v1:
 - Remove timeout configuration.
 - Modify the binding file to add hwlocks.
---
 Documentation/devicetree/bindings/mfd/syscon.txt |    8 ++++++++
 drivers/mfd/syscon.c                             |   21 +++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/syscon.txt b/Documentation/devicetree/bindings/mfd/syscon.txt
index 8b92d45..25d9e9c 100644
--- a/Documentation/devicetree/bindings/mfd/syscon.txt
+++ b/Documentation/devicetree/bindings/mfd/syscon.txt
@@ -16,9 +16,17 @@ Required properties:
 Optional property:
 - reg-io-width: the size (in bytes) of the IO accesses that should be
   performed on the device.
+- hwlocks: reference to a phandle of a hardware spinlock provider node.
 
 Examples:
 gpr: iomuxc-gpr@20e0000 {
 	compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
 	reg = <0x020e0000 0x38>;
+	hwlocks = <&hwlock1 1>;
+};
+
+hwlock1: hwspinlock@40500000 {
+	...
+	reg = <0x40500000 0x1000>;
+	#hwlock-cells = <1>;
 };
diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index b93fe4c..5b67dbc 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -13,6 +13,7 @@
  */
 
 #include <linux/err.h>
+#include <linux/hwspinlock.h>
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/list.h>
@@ -87,6 +88,26 @@ static struct syscon *of_syscon_register(struct device_node *np)
 	if (ret)
 		reg_io_width = 4;
 
+	ret = of_hwspin_lock_get_id(np, 0);
+	if (ret > 0) {
+		syscon_config.hwlock_id = ret;
+		syscon_config.hwlock_mode = HWLOCK_IRQSTATE;
+	} else {
+		switch (ret) {
+		case -ENOENT:
+			/* Ignore missing hwlock, it's optional. */
+			break;
+		case 0:
+			ret = -EINVAL;
+			/* fall-through */
+		default:
+			pr_err("Failed to retrieve valid hwlock: %d\n", ret);
+			/* fall-through */
+		case -EPROBE_DEFER:
+			goto err_regmap;
+		}
+	}
+
 	syscon_config.reg_stride = reg_io_width;
 	syscon_config.val_bits = reg_io_width * 8;
 	syscon_config.max_register = resource_size(&res) - reg_io_width;
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related

* [PATCH] powerpc/kmcent2: update the ethernet nodes to the internal switch
From: Valentin Longchamp @ 2017-11-30  9:16 UTC (permalink / raw)
  To: oss-fOR+EgIDQEHk1uMJSBkQmQ, linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, Valentin Longchamp

The fman driver only calls of_get_phy_node, ignoring the
phy-connection-type field. While we're at it, update the fixed link to
the new more readable way to specify it.

Signed-off-by: Valentin Longchamp <valentin.longchamp-SkAbAL50j+5BDgjK7y7TUQ@public.gmane.org>
---
 arch/powerpc/boot/dts/fsl/kmcent2.dts | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/boot/dts/fsl/kmcent2.dts b/arch/powerpc/boot/dts/fsl/kmcent2.dts
index 5922c1ea0e96..b53ff8f71474 100644
--- a/arch/powerpc/boot/dts/fsl/kmcent2.dts
+++ b/arch/powerpc/boot/dts/fsl/kmcent2.dts
@@ -214,13 +214,19 @@
 
 		fman@400000 {
 			ethernet@e0000 {
-				fixed-link = <0 1 1000 0 0>;
-				phy-connection-type = "sgmii";
+				phy-mode = "sgmii";
+				fixed-link {
+					speed = <1000>;
+					full-duplex;
+				};
 			};
 
 			ethernet@e2000 {
-				fixed-link = <1 1 1000 0 0>;
-				phy-connection-type = "sgmii";
+				phy-mode = "sgmii";
+				fixed-link {
+					speed = <1000>;
+					full-duplex;
+				};
 			};
 
 			ethernet@e4000 {
-- 
2.14.3
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related

* Re: [PATCH v2 25/35] nds32: Build infrastructure
From: Greentime Hu @ 2017-11-30  9:29 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Arnd Bergmann, Greentime, Linux Kernel Mailing List, linux-arch,
	Thomas Gleixner, Jason Cooper, Marc Zyngier, Rob Herring,
	Networking, Vincent Chen, DTML, Al Viro, David Howells,
	Will Deacon, Daniel Lezcano, linux-serial@vger.kernel.org,
	Vincent Chen
In-Reply-To: <CAMuHMdWXe-1=-98V=AcUzu-MiEaAqPQi2HZ6OTS=8sXY3XNhjw@mail.gmail.com>

2017-11-30 15:52 GMT+08:00 Geert Uytterhoeven <geert@linux-m68k.org>:
> On Thu, Nov 30, 2017 at 6:48 AM, Greentime Hu <green.hu@gmail.com> wrote:
>> 2017-11-30 4:27 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
>>> On Wed, Nov 29, 2017 at 3:10 PM, Greentime Hu <green.hu@gmail.com> wrote:
>>>> 2017-11-29 19:57 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
>>>>> On Wed, Nov 29, 2017 at 12:39 PM, Greentime Hu <green.hu@gmail.com> wrote:
>> I think I can use this name "CPU_V3" for all nds32 v3 compatible cpu.
>> It will be implemented like this.
>>
>> config HWZOL
>>         bool "hardware zero overhead loop support"
>>         depends on CPU_D10 || CPU_D15
>>         default n
>>         help
>>           A set of Zero-Overhead Loop mechanism is provided to reduce the
>>           instruction fetch and execution overhead of loop-control instructions.
>>           It will save 3 registers($LB, $LC, $LE) for context saving if say Y.
>>           You don't need to save these registers if you can make sure your user
>>           program doesn't use these registers.
>>
>>           If unsure, say N.
>>
>> config CPU_CACHE_NONALIASING
>>         bool "Non-aliasing cache"
>>         depends on !CPU_N10 && !CPU_D10
>>         default n
>>         help
>>           If this CPU is using VIPT data cache and its cache way size is larger
>>           than page size, say N. If it is using PIPT data cache, say Y.
>>
>>           If unsure, say N.
>
> I still think it will be easier to revert the logic, and have
> CPU_CACHE_ALIASING.
>

Thanks Geert

I will implement it like this.

config HWZOL
        bool "hardware zero overhead loop support"
        depends on CPU_D10 || CPU_D15
        default n
        help
          A set of Zero-Overhead Loop mechanism is provided to reduce the
          instruction fetch and execution overhead of loop-control instructions.
          It will save 3 registers($LB, $LC, $LE) for context saving if say Y.
          You don't need to save these registers if you can make sure your user
          program doesn't use these registers.

          If unsure, say N.

config CPU_CACHE_ALIASING
        bool "Aliasing cache"
        depends on CPU_N10 || CPU_D10 || CPU_N13 || CPU_V3
        default y
        help
          If this CPU is using VIPT data cache and its cache way size is larger
          than page size, say Y. If it is using PIPT data cache, say N.

          If unsure, say Y.

choice
        prompt "CPU type"
        default CPU_V3
config CPU_N15
        bool "AndesCore N15"
config CPU_N13
        bool "AndesCore N13"
        select CPU_CACHE_ALIASING if ANDES_PAGE_SIZE_4KB
config CPU_N10
        bool "AndesCore N10"
        select CPU_CACHE_ALIASING
config CPU_D15
        bool "AndesCore D15"
config CPU_D10
        bool "AndesCore D10"
        select CPU_CACHE_ALIASING
config CPU_V3
        bool "AndesCore v3 compatible"
        select ANDES_PAGE_SIZE_8KB
endchoice

^ permalink raw reply

* Re: [PATCH v2 25/35] nds32: Build infrastructure
From: Arnd Bergmann @ 2017-11-30  9:30 UTC (permalink / raw)
  To: Greentime Hu
  Cc: Geert Uytterhoeven, Greentime, Linux Kernel Mailing List,
	linux-arch, Thomas Gleixner, Jason Cooper, Marc Zyngier,
	Rob Herring, Networking, Vincent Chen, DTML, Al Viro,
	David Howells, Will Deacon, Daniel Lezcano,
	linux-serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Vincent Chen
In-Reply-To: <CAEbi=3cTkbt9i7XPXMnY1D6qtbebDW1x8sFVsgqhq-nApAx5mA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

On Thu, Nov 30, 2017 at 6:48 AM, Greentime Hu <green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 2017-11-30 4:27 GMT+08:00 Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>:
>> On Wed, Nov 29, 2017 at 3:10 PM, Greentime Hu <green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> 2017-11-29 19:57 GMT+08:00 Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>:

>> When you put them in a sorted list like I mentioned for simplicity, you
>> could reduce the confusion by naming them differently, e.g.
>> CONFIG_CPU_N10_OR_NEWER.
>>
>> Having only the CPU_CACHE_NONALIASING option is fine if you
>> never need to make any other decisions based on the CPU core
>> type, but then the help text should describe specifically which cases
>> are affected (N10/N13/D13 with 4K page size), and you can decide to
>> hide the option and make it always-on when using 8K page size.
>>
>>        Arnd
>
>
> Hi, Arnd:
>
> I think I can use this name "CPU_V3" for all nds32 v3 compatible cpu.
> It will be implemented like this.

I think I'm still a bit confused about the relation between CPU cores
and architecture levels. Is it correct to say that there are orthogonal,
and that you can have e.g. an N10 core implementing either nds32v2
or nds32v3?

There is nothing wrong with that of course, it's just not what I
expected from having worked with other architectures.

I also see that GCC has no pipeline specific optimizations for
specific cores, it just understands the differences between the
architecture levels, so at least today there is way to pass e.g.
"-march=nds32v2 -mtune=d15" to generate code that would
work on both v2 and v3 but be optimized for d15.

> config HWZOL
>         bool "hardware zero overhead loop support"
>         depends on CPU_D10 || CPU_D15
>         default n
>         help
>           A set of Zero-Overhead Loop mechanism is provided to reduce the
>           instruction fetch and execution overhead of loop-control instructions.
>           It will save 3 registers($LB, $LC, $LE) for context saving if say Y.
>           You don't need to save these registers if you can make sure your user
>           program doesn't use these registers.
>
>           If unsure, say N.
>
> config CPU_CACHE_NONALIASING
>         bool "Non-aliasing cache"
>         depends on !CPU_N10 && !CPU_D10
>         default n
>         help
>           If this CPU is using VIPT data cache and its cache way size is larger
>           than page size, say N. If it is using PIPT data cache, say Y.
>
>           If unsure, say N.

This looks ok, yes, but as Geert said, it would seem more intuitive to
write it as

config CPU_CACHE_ALIASING
         bool "Aliasing VIPT cache"
         depends on CPU_N10 || CPU_D10

> choice
>         prompt "CPU type"
>         default CPU_V3
> config CPU_N15
>         bool "AndesCore N15"
>         select CPU_CACHE_NONALIASING
> config CPU_N13
>         bool "AndesCore N13"
>         select CPU_CACHE_NONALIASING if ANDES_PAGE_SIZE_8KB
> config CPU_N10
>         bool "AndesCore N10"
> config CPU_D15
>         bool "AndesCore D15"
>         select CPU_CACHE_NONALIASING
> config CPU_D10
>         bool "AndesCore D10"
> config CPU_V3
>         bool "AndesCore v3 compatible"
>         select ANDES_PAGE_SIZE_4KB
> endchoice

Two points here:

- Generally you should not mix 'select' and 'depends on' like this.
  Either you make the cache aliasing a user visible option that
  uses 'depends on' with a combination of CPU cores, or you
  make it a hidden option (with no string after the "bool" keyword)
  that always gets selected from the per-cpu options.

- There is a  little-known trick with choice statements that allows
  you to use 'tristate' instead of 'bool' in the choice. In that case,
  you can enable multiple options together as long as all of them
  are 'm'.

         Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH] powerpc/kmcent2: update the ethernet nodes to the internal switch
From: Valentin Longchamp @ 2017-11-30  9:31 UTC (permalink / raw)
  To: valentin.longchamp-SkAbAL50j+5BDgjK7y7TUQ@public.gmane.org,
	oss-fOR+EgIDQEHk1uMJSBkQmQ@public.gmane.org,
	linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <20171130091621.6997-1-valentin.longchamp-SkAbAL50j+5BDgjK7y7TUQ@public.gmane.org>

From: Valentin Longchamp <valentin.longchamp-SkAbAL50j+5BDgjK7y7TUQ@public.gmane.org>
Sent: Thursday, November 30, 2017 10:16
To: oss-fOR+EgIDQEHk1uMJSBkQmQ@public.gmane.org; linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; valentin.longchamp-SkAbAL50j+5BDgjK7y7TUQ@public.gmane.org
Subject: [PATCH] powerpc/kmcent2: update the ethernet nodes to the internal switch

Sorry for the noise, please ignore this patch, I've messed up my rebase with our internal tree.

The fman driver only calls of_get_phy_node, ignoring the
phy-connection-type field. While we're at it, update the fixed link to
the new more readable way to specify it.

Signed-off-by: Valentin Longchamp <valentin.longchamp-SkAbAL50j+5BDgjK7y7TUQ@public.gmane.org>
---
 arch/powerpc/boot/dts/fsl/kmcent2.dts | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/boot/dts/fsl/kmcent2.dts b/arch/powerpc/boot/dts/fsl/kmcent2.dts
index 5922c1ea0e96..b53ff8f71474 100644
--- a/arch/powerpc/boot/dts/fsl/kmcent2.dts
+++ b/arch/powerpc/boot/dts/fsl/kmcent2.dts
@@ -214,13 +214,19 @@

                fman@400000 {
                        ethernet@e0000 {
-                               fixed-link = <0 1 1000 0 0>;
-                               phy-connection-type = "sgmii";
+                               phy-mode = "sgmii";
+                               fixed-link {
+                                       speed = <1000>;
+                                       full-duplex;
+                               };
                        };

                        ethernet@e2000 {
-                               fixed-link = <1 1 1000 0 0>;
-                               phy-connection-type = "sgmii";
+                               phy-mode = "sgmii";
+                               fixed-link {
+                                       speed = <1000>;
+                                       full-duplex;
+                               };
                        };

                        ethernet@e4000 {
--
2.14.3

    --
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related

* [PATCH] powerpc/kmcent2: update the ethernet nodes to use phy-mode
From: Valentin Longchamp @ 2017-11-30  9:33 UTC (permalink / raw)
  To: oss-fOR+EgIDQEHk1uMJSBkQmQ, linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, Valentin Longchamp

The fman driver only calls of_get_phy_node, ignoring the
phy-connection-type field. While we're at it, update the fixed link to
the new more readable way to specify it and set phy-mode to rgmii-id for
the node with a phy.

Signed-off-by: Valentin Longchamp <valentin.longchamp-SkAbAL50j+5BDgjK7y7TUQ@public.gmane.org>
---
 arch/powerpc/boot/dts/fsl/kmcent2.dts | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/boot/dts/fsl/kmcent2.dts b/arch/powerpc/boot/dts/fsl/kmcent2.dts
index 5922c1ea0e96..095c9a661a08 100644
--- a/arch/powerpc/boot/dts/fsl/kmcent2.dts
+++ b/arch/powerpc/boot/dts/fsl/kmcent2.dts
@@ -214,13 +214,19 @@
 
 		fman@400000 {
 			ethernet@e0000 {
-				fixed-link = <0 1 1000 0 0>;
-				phy-connection-type = "sgmii";
+				phy-mode = "sgmii";
+				fixed-link {
+					speed = <1000>;
+					full-duplex;
+				};
 			};
 
 			ethernet@e2000 {
-				fixed-link = <1 1 1000 0 0>;
-				phy-connection-type = "sgmii";
+				phy-mode = "sgmii";
+				fixed-link {
+					speed = <1000>;
+					full-duplex;
+				};
 			};
 
 			ethernet@e4000 {
@@ -233,7 +239,7 @@
 
 			ethernet@e8000 {
 				phy-handle = <&front_phy>;
-				phy-connection-type = "rgmii";
+				phy-mode = "rgmii-id";
 			};
 
 			mdio0: mdio@fc000 {
-- 
2.14.3
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related

* Re: [PATCH] rtc: add mxc driver for i.MX53
From: Sascha Hauer @ 2017-11-30  9:50 UTC (permalink / raw)
  To: linux-kernel-dev-QonKdJ6Bx35Wk0Htik3J/w
  Cc: Alessandro Zummo, Alexandre Belloni, Mark Rutland,
	open list:REAL TIME CLOCK (RTC) SUBSYSTEM, Patrick Bruenn,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Juergen Borleis, open list, Russell King, Noel Vellemans,
	Rob Herring, Sascha Hauer, Fabio Estevam, Shawn Guo,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <20171128073927.12035-1-linux-kernel-dev-QonKdJ6Bx35Wk0Htik3J/w@public.gmane.org>

On Tue, Nov 28, 2017 at 08:39:27AM +0100, linux-kernel-dev-QonKdJ6Bx35Wk0Htik3J/w@public.gmane.org wrote:
> From: Patrick Bruenn <p.bruenn-QonKdJ6Bx35Wk0Htik3J/w@public.gmane.org>
> 
> Neither rtc-imxdi nor rtc-mxc are compatible with i.MX53.
> Add a modernized version of mxc_v2 from here:
> http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/drivers/rtc/rtc-mxc_v2.c?h=imx_2.6.35_11.09.01
> 
> Changes to that version:
> - updated to v4.15-rc1
> - removed ioctl()
> - removed proc()
> - removed manual(redundant) enable_irq flag
> 
> Signed-off-by: Patrick Bruenn <p.bruenn-QonKdJ6Bx35Wk0Htik3J/w@public.gmane.org>
> 
> ---
> 
> Open issues:
> - driver naming, should it be merged with rtc-mxc.c ?
> - document DT binding "fsl,imx53-rtc" accordingly
> - Should unused defines be removed or kept for someone else to be
>   useful?
> - Is the use of __raw_readl/writel() correct? Should it be replaced with
>   readl/writel()?
> - suspend/resume() seems different to existing rtc-mxc.c, should I apply
>   the pattern from rtc-mxc.c?
> - On Shawns tree imx53.dtsi has been reverted already[1][2]. Should I split
>   the imx53.dtsi change into a separate patch based on his tree? Or can
>   we still stop the full revert and just remove the imx25-rtc compatible?
>   I am not in a hurry, so we could just wait until the revert landed in
>   Linus tree. Whatever you think is best.
> 
> [1] https://www.spinics.net/lists/arm-kernel/msg617113.html
> [2] commit ee76f7729babd2700afd6f3874449d8084dd85ea
> 
> To: Alessandro Zummo <a.zummo-BfzFCNDTiLLj+vYz1yj4TQ@public.gmane.org>
> To: Alexandre Belloni <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
> Cc: linux-rtc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org (open list:REAL TIME CLOCK (RTC) SUBSYSTEM)
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
> Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org (open list)
> Cc: Fabio Estevam <fabio.estevam-3arQi8VN3Tc@public.gmane.org>
> Cc: Juergen Borleis <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Cc: Noel Vellemans <Noel.Vellemans-8UENEgx6w+makBO8gow8eQ@public.gmane.org>
> Cc: Shawn Guo <shawnguo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> (maintainer:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)
> Cc: Russell King <linux-I+IVW8TIWO2tmTQ+vhA3Yw@public.gmane.org> (maintainer:ARM PORT)
> Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org (moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)
> ---
>  arch/arm/boot/dts/imx53.dtsi |   2 +-
>  drivers/rtc/Makefile         |   1 +
>  drivers/rtc/rtc-mxc_v2.c     | 531 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 533 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/rtc/rtc-mxc_v2.c
> 
> diff --git a/arch/arm/boot/dts/imx53.dtsi b/arch/arm/boot/dts/imx53.dtsi
> index 589a67c5f796..3d1a55e11ea8 100644
> --- a/arch/arm/boot/dts/imx53.dtsi
> +++ b/arch/arm/boot/dts/imx53.dtsi
> @@ -434,7 +434,7 @@
>  			};
>  
>  			srtc: srtc@53fa4000 {
> -				compatible = "fsl,imx53-rtc", "fsl,imx25-rtc";
> +				compatible = "fsl,imx53-rtc";
>  				reg = <0x53fa4000 0x4000>;
>  				interrupts = <24>;
>  				interrupt-parent = <&tzic>;
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index f2f50c11dc38..fb3dc458c185 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -106,6 +106,7 @@ obj-$(CONFIG_RTC_DRV_MT6397)	+= rtc-mt6397.o
>  obj-$(CONFIG_RTC_DRV_MT7622)	+= rtc-mt7622.o
>  obj-$(CONFIG_RTC_DRV_MV)	+= rtc-mv.o
>  obj-$(CONFIG_RTC_DRV_MXC)	+= rtc-mxc.o
> +obj-$(CONFIG_RTC_DRV_MXC)	+= rtc-mxc_v2.o
>  obj-$(CONFIG_RTC_DRV_NUC900)	+= rtc-nuc900.o
>  obj-$(CONFIG_RTC_DRV_OMAP)	+= rtc-omap.o
>  obj-$(CONFIG_RTC_DRV_OPAL)	+= rtc-opal.o
> diff --git a/drivers/rtc/rtc-mxc_v2.c b/drivers/rtc/rtc-mxc_v2.c
> new file mode 100644
> index 000000000000..5049b521b38e
> --- /dev/null
> +++ b/drivers/rtc/rtc-mxc_v2.c
> @@ -0,0 +1,531 @@
> +/*
> + * Copyright (C) 2004-2011 Freescale Semiconductor, Inc. All Rights Reserved.
> + */
> +
> +/*
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +/*
> + * Implementation based on rtc-ds1553.c
> + */
> +
> +/*!
> + * @defgroup RTC Real Time Clock (RTC) Driver for i.MX53
> + */
> +/*!
> + * @file rtc-mxc_v2.c
> + * @brief Real Time Clock interface
> + *
> + * This file contains Real Time Clock interface for Linux.
> + *
> + * @ingroup RTC
> + */
> +
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/rtc.h>
> +#include <linux/module.h>
> +#include <linux/fs.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/uaccess.h>
> +#include <linux/io.h>
> +//#include <linux/mxc_srtc.h>
> +#define RTC_READ_TIME_47BIT	_IOR('p', 0x20, unsigned long long)
> +/* blocks until LPSCMR is set, returns difference */
> +#define RTC_WAIT_TIME_SET	_IOR('p', 0x21, int64_t)
> +
> +#define SRTC_LPSCLR_LLPSC_LSH	17	/* start bit for LSB time value */
> +
> +#define SRTC_LPPDR_INIT       0x41736166	/* init for glitch detect */
> +
> +#define SRTC_LPCR_SWR_LP      (1 << 0)	/* lp software reset */
> +#define SRTC_LPCR_EN_LP       (1 << 3)	/* lp enable */
> +#define SRTC_LPCR_WAE         (1 << 4)	/* lp wakeup alarm enable */
> +#define SRTC_LPCR_SAE         (1 << 5)	/* lp security alarm enable */
> +#define SRTC_LPCR_SI          (1 << 6)	/* lp security interrupt enable */
> +#define SRTC_LPCR_ALP         (1 << 7)	/* lp alarm flag */
> +#define SRTC_LPCR_LTC         (1 << 8)	/* lp lock time counter */
> +#define SRTC_LPCR_LMC         (1 << 9)	/* lp lock monotonic counter */
> +#define SRTC_LPCR_SV          (1 << 10)	/* lp security violation */
> +#define SRTC_LPCR_NSA         (1 << 11)	/* lp non secure access */
> +#define SRTC_LPCR_NVEIE       (1 << 12)	/* lp non valid state exit int en */
> +#define SRTC_LPCR_IEIE        (1 << 13)	/* lp init state exit int enable */
> +#define SRTC_LPCR_NVE         (1 << 14)	/* lp non valid state exit bit */
> +#define SRTC_LPCR_IE          (1 << 15)	/* lp init state exit bit */
> +
> +#define SRTC_LPCR_ALL_INT_EN (SRTC_LPCR_WAE | SRTC_LPCR_SAE | \
> +			      SRTC_LPCR_SI | SRTC_LPCR_ALP | \
> +			      SRTC_LPCR_NVEIE | SRTC_LPCR_IEIE)
> +
> +#define SRTC_LPSR_TRI         (1 << 0)	/* lp time read invalidate */
> +#define SRTC_LPSR_PGD         (1 << 1)	/* lp power supply glitc detected */
> +#define SRTC_LPSR_CTD         (1 << 2)	/* lp clock tampering detected */
> +#define SRTC_LPSR_ALP         (1 << 3)	/* lp alarm flag */
> +#define SRTC_LPSR_MR          (1 << 4)	/* lp monotonic counter rollover */
> +#define SRTC_LPSR_TR          (1 << 5)	/* lp time rollover */
> +#define SRTC_LPSR_EAD         (1 << 6)	/* lp external alarm detected */
> +#define SRTC_LPSR_IT0         (1 << 7)	/* lp IIM throttle */
> +#define SRTC_LPSR_IT1         (1 << 8)
> +#define SRTC_LPSR_IT2         (1 << 9)
> +#define SRTC_LPSR_SM0         (1 << 10)	/* lp security mode */
> +#define SRTC_LPSR_SM1         (1 << 11)
> +#define SRTC_LPSR_STATE_LP0   (1 << 12)	/* lp state */
> +#define SRTC_LPSR_STATE_LP1   (1 << 13)
> +#define SRTC_LPSR_NVES        (1 << 14)	/* lp non-valid state exit status */
> +#define SRTC_LPSR_IES         (1 << 15)	/* lp init state exit status */
> +
> +#define MAX_PIE_NUM     15
> +#define MAX_PIE_FREQ    32768
> +#define MIN_PIE_FREQ	1
> +
> +#define SRTC_PI0         (1 << 0)
> +#define SRTC_PI1         (1 << 1)
> +#define SRTC_PI2         (1 << 2)
> +#define SRTC_PI3         (1 << 3)
> +#define SRTC_PI4         (1 << 4)
> +#define SRTC_PI5         (1 << 5)
> +#define SRTC_PI6         (1 << 6)
> +#define SRTC_PI7         (1 << 7)
> +#define SRTC_PI8         (1 << 8)
> +#define SRTC_PI9         (1 << 9)
> +#define SRTC_PI10        (1 << 10)
> +#define SRTC_PI11        (1 << 11)
> +#define SRTC_PI12        (1 << 12)
> +#define SRTC_PI13        (1 << 13)
> +#define SRTC_PI14        (1 << 14)
> +#define SRTC_PI15        (1 << 15)
> +
> +#define PIT_ALL_ON      (SRTC_PI1 | SRTC_PI2 | SRTC_PI3 | \
> +			SRTC_PI4 | SRTC_PI5 | SRTC_PI6 | SRTC_PI7 | \
> +			SRTC_PI8 | SRTC_PI9 | SRTC_PI10 | SRTC_PI11 | \
> +			SRTC_PI12 | SRTC_PI13 | SRTC_PI14 | SRTC_PI15)
> +
> +#define SRTC_SWR_HP      (1 << 0)	/* hp software reset */
> +#define SRTC_EN_HP       (1 << 3)	/* hp enable */
> +#define SRTC_TS          (1 << 4)	/* time synchronize hp with lp */
> +
> +#define SRTC_IE_AHP      (1 << 16)	/* Alarm HP Interrupt Enable bit */
> +#define SRTC_IE_WDHP     (1 << 18)	/* Write Done HP Interrupt Enable bit */
> +#define SRTC_IE_WDLP     (1 << 19)	/* Write Done LP Interrupt Enable bit */
> +
> +#define SRTC_ISR_AHP     (1 << 16)	/* interrupt status: alarm hp */
> +#define SRTC_ISR_WDHP    (1 << 18)	/* interrupt status: write done hp */
> +#define SRTC_ISR_WDLP    (1 << 19)	/* interrupt status: write done lp */
> +#define SRTC_ISR_WPHP    (1 << 20)	/* interrupt status: write pending hp */
> +#define SRTC_ISR_WPLP    (1 << 21)	/* interrupt status: write pending lp */
> +
> +#define SRTC_LPSCMR	0x00	/* LP Secure Counter MSB Reg */
> +#define SRTC_LPSCLR	0x04	/* LP Secure Counter LSB Reg */
> +#define SRTC_LPSAR	0x08	/* LP Secure Alarm Reg */
> +#define SRTC_LPSMCR	0x0C	/* LP Secure Monotonic Counter Reg */
> +#define SRTC_LPCR	0x10	/* LP Control Reg */
> +#define SRTC_LPSR	0x14	/* LP Status Reg */
> +#define SRTC_LPPDR	0x18	/* LP Power Supply Glitch Detector Reg */
> +#define SRTC_LPGR	0x1C	/* LP General Purpose Reg */
> +#define SRTC_HPCMR	0x20	/* HP Counter MSB Reg */
> +#define SRTC_HPCLR	0x24	/* HP Counter LSB Reg */
> +#define SRTC_HPAMR	0x28	/* HP Alarm MSB Reg */
> +#define SRTC_HPALR	0x2C	/* HP Alarm LSB Reg */
> +#define SRTC_HPCR	0x30	/* HP Control Reg */
> +#define SRTC_HPISR	0x34	/* HP Interrupt Status Reg */
> +#define SRTC_HPIENR	0x38	/* HP Interrupt Enable Reg */
> +
> +#define SRTC_SECMODE_MASK	0x3	/* the mask of SRTC security mode */
> +#define SRTC_SECMODE_LOW	0x0	/* Low Security */
> +#define SRTC_SECMODE_MED	0x1	/* Medium Security */
> +#define SRTC_SECMODE_HIGH	0x2	/* High Security */
> +#define SRTC_SECMODE_RESERVED	0x3	/* Reserved */
> +
> +struct rtc_drv_data {
> +	struct rtc_device *rtc;
> +	void __iomem *ioaddr;
> +	int irq;
> +	struct clk *clk;
> +};
> +
> +static DEFINE_SPINLOCK(rtc_lock);

The lock should be a member of your driver data struct.

> +
> +/*!
> + * This function does write synchronization for writes to the lp srtc block.
> + * To take care of the asynchronous CKIL clock, all writes from the IP domain
> + * will be synchronized to the CKIL domain.
> + */

Please drop this exclamation mark at the beginning of comments. If
anything, use kerneldoc style for the function descriptions.

> +static inline void rtc_write_sync_lp(void __iomem *ioaddr)
> +{
> +	unsigned int i, count;
> +	/* Wait for 3 CKIL cycles */
> +	for (i = 0; i < 3; i++) {
> +		count = readl(ioaddr + SRTC_LPSCLR);
> +		while ((readl(ioaddr + SRTC_LPSCLR)) == count)
> +			;

This becomes an infinite loop when the hardware is not working as
expected. You should avoid that.

> +	}
> +}
> +
> +/*!
> + * This function updates the RTC alarm registers and then clears all the
> + * interrupt status bits.
> + *
> + * @param  alrm         the new alarm value to be updated in the RTC
> + *
> + * @return  0 if successful; non-zero otherwise.
> + */
> +static int rtc_update_alarm(struct device *dev, struct rtc_time *alrm)
> +{

mxc_rtc_update_alarm please like the other device specific functions
please. Same for rtc_write_sync_lp().

> +	struct rtc_drv_data *pdata = dev_get_drvdata(dev);
> +	void __iomem *ioaddr = pdata->ioaddr;
> +	struct rtc_time alarm_tm, now_tm;
> +	unsigned long now, time;
> +	int ret;
> +
> +	now = __raw_readl(ioaddr + SRTC_LPSCMR);
> +	rtc_time_to_tm(now, &now_tm);
> +
> +	alarm_tm.tm_year = now_tm.tm_year;
> +	alarm_tm.tm_mon = now_tm.tm_mon;
> +	alarm_tm.tm_mday = now_tm.tm_mday;
> +
> +	alarm_tm.tm_hour = alrm->tm_hour;
> +	alarm_tm.tm_min = alrm->tm_min;
> +	alarm_tm.tm_sec = alrm->tm_sec;
> +
> +	rtc_tm_to_time(&now_tm, &now);
> +	rtc_tm_to_time(&alarm_tm, &time);
> +
> +	if (time < now) {
> +		time += 60 * 60 * 24;
> +		rtc_time_to_tm(time, &alarm_tm);
> +	}
> +	ret = rtc_tm_to_time(&alarm_tm, &time);
> +
> +	__raw_writel(time, ioaddr + SRTC_LPSAR);
> +
> +	/* clear alarm interrupt status bit */
> +	__raw_writel(SRTC_LPSR_ALP, ioaddr + SRTC_LPSR);
> +
> +	return ret;
> +}
> +
> +/*!
> + * This function is the RTC interrupt service routine.
> + *
> + * @param  irq          RTC IRQ number
> + * @param  dev_id       device ID which is not used
> + *
> + * @return IRQ_HANDLED as defined in the include/linux/interrupt.h file.
> + */
> +static irqreturn_t mxc_rtc_interrupt(int irq, void *dev_id)
> +{
> +	struct platform_device *pdev = dev_id;
> +	struct rtc_drv_data *pdata = platform_get_drvdata(pdev);
> +	void __iomem *ioaddr = pdata->ioaddr;
> +	u32 lp_status, lp_cr;
> +	u32 events = 0;
> +
> +	lp_status = __raw_readl(ioaddr + SRTC_LPSR);
> +	lp_cr = __raw_readl(ioaddr + SRTC_LPCR);
> +
> +	/* update irq data & counter */
> +	if (lp_status & SRTC_LPSR_ALP) {
> +		if (lp_cr & SRTC_LPCR_ALP)
> +			events |= (RTC_AF | RTC_IRQF);
> +
> +		/* disable further lp alarm interrupts */
> +		lp_cr &= ~(SRTC_LPCR_ALP | SRTC_LPCR_WAE);
> +	}
> +
> +	/* Update interrupt enables */
> +	__raw_writel(lp_cr, ioaddr + SRTC_LPCR);
> +
> +	/* clear interrupt status */
> +	__raw_writel(lp_status, ioaddr + SRTC_LPSR);
> +
> +	rtc_write_sync_lp(ioaddr);
> +	rtc_update_irq(pdata->rtc, 1, events);
> +	return IRQ_HANDLED;
> +}

What about locking? You lock the read/modify/write operation of the LPCR
register below, and so you should do here.

> +
> +/*!
> + * This function reads the current RTC time into tm in Gregorian date.
> + *
> + * @param  tm           contains the RTC time value upon return
> + *
> + * @return  0 if successful; non-zero otherwise.
> + */
> +static int mxc_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct rtc_drv_data *pdata = dev_get_drvdata(dev);
> +	time_t now = readl(pdata->ioaddr + SRTC_LPSCMR);
> +
> +	rtc_time_to_tm(now, tm);
> +	return rtc_valid_tm(tm);
> +}
> +
> +/*!
> + * This function sets the internal RTC time based on tm in Gregorian date.
> + *
> + * @param  tm           the time value to be set in the RTC
> + *
> + * @return  0 if successful; non-zero otherwise.
> + */
> +static int mxc_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct rtc_drv_data *pdata = dev_get_drvdata(dev);
> +	time64_t time = rtc_tm_to_time64(tm);
> +
> +	if (time > UINT_MAX) {
> +		dev_warn(dev, "time_t has overflow\n");
> +	}

SIGH. Who builds such a RTC in the 21st century?

Shouldn't this return an error?

> +
> +	writel(time, pdata->ioaddr + SRTC_LPSCMR);
> +	rtc_write_sync_lp(pdata->ioaddr);
> +	return 0;
> +}
> +
> +/*!
> + * This function reads the current alarm value into the passed in \b alrm
> + * argument. It updates the \b alrm's pending field value based on the whether
> + * an alarm interrupt occurs or not.
> + *
> + * @param  alrm         contains the RTC alarm value upon return
> + *
> + * @return  0 if successful; non-zero otherwise.
> + */
> +static int mxc_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +	struct rtc_drv_data *pdata = dev_get_drvdata(dev);
> +	void __iomem *ioaddr = pdata->ioaddr;
> +
> +	rtc_time_to_tm(__raw_readl(ioaddr + SRTC_LPSAR), &alrm->time);
> +	alrm->pending =
> +	    ((__raw_readl(ioaddr + SRTC_LPSR) & SRTC_LPSR_ALP) != 0) ? 1 : 0;

if/else would be much easier to read here.

> +
> +	return 0;
> +}
> +
> +static int mxc_rtc_alarm_irq_enable(struct device *dev, unsigned int enable)
> +{
> +	struct rtc_drv_data *pdata = dev_get_drvdata(dev);
> +	unsigned long lock_flags = 0;
> +	u32 lp_cr;
> +
> +	spin_lock_irqsave(&rtc_lock, lock_flags);
> +	lp_cr = __raw_readl(pdata->ioaddr + SRTC_LPCR);
> +
> +	if (enable)
> +		lp_cr |= (SRTC_LPCR_ALP | SRTC_LPCR_WAE);
> +	else
> +		lp_cr &= ~(SRTC_LPCR_ALP | SRTC_LPCR_WAE);
> +
> +	__raw_writel(lp_cr, pdata->ioaddr + SRTC_LPCR);
> +	spin_unlock_irqrestore(&rtc_lock, lock_flags);
> +	return 0;
> +}
> +
> +/*!
> + * This function sets the RTC alarm based on passed in alrm.
> + *
> + * @param  alrm         the alarm value to be set in the RTC
> + *
> + * @return  0 if successful; non-zero otherwise.
> + */
> +static int mxc_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +	struct rtc_drv_data *pdata = dev_get_drvdata(dev);
> +	void __iomem *ioaddr = pdata->ioaddr;
> +	int ret;
> +
> +	ret = rtc_update_alarm(dev, &alrm->time);
> +	if (!ret)
> +		mxc_rtc_alarm_irq_enable(dev, alrm->enabled);
> +
> +	rtc_write_sync_lp(ioaddr);
> +	return ret;
> +}
> +
> +/*!
> + * The RTC driver structure
> + */
> +static const struct rtc_class_ops mxc_rtc_ops = {
> +	.read_time = mxc_rtc_read_time,
> +	.set_time = mxc_rtc_set_time,
> +	.read_alarm = mxc_rtc_read_alarm,
> +	.set_alarm = mxc_rtc_set_alarm,
> +	.alarm_irq_enable = mxc_rtc_alarm_irq_enable,
> +};
> +
> +/*! MXC RTC Power management control */
> +static int mxc_rtc_probe(struct platform_device *pdev)
> +{
> +	struct timespec tv;
> +	struct resource *res;
> +	struct rtc_drv_data *pdata = NULL;

No need to initialize.

> +	void __iomem *ioaddr;
> +	int ret = 0;
> +
> +	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENODEV;
> +
> +	pdata->ioaddr = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(pdata->ioaddr))
> +		return PTR_ERR(pdata->ioaddr);
> +
> +	ioaddr = pdata->ioaddr;
> +
> +	pdata->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(pdata->clk)) {
> +		dev_err(&pdev->dev, "unable to get rtc clock!\n");
> +		return PTR_ERR(pdata->clk);
> +	}
> +	ret = clk_prepare_enable(pdata->clk);
> +	if (ret)
> +		return ret;
> +
> +	/* Configure and enable the RTC */
> +	pdata->irq = platform_get_irq(pdev, 0);
> +	if (pdata->irq >= 0
> +	    && devm_request_irq(&pdev->dev, pdata->irq, mxc_rtc_interrupt,
> +				IRQF_SHARED, pdev->name, pdev) < 0) {
> +		dev_warn(&pdev->dev, "interrupt not available.\n");
> +		pdata->irq = -1;
> +	}

I would just make the interrupt mandatory. I see no valid reason why it
shouldn't be available and it makes your driver logic simpler when you
can just assume it's available.

> +
> +	if (pdata->irq >= 0)
> +		device_init_wakeup(&pdev->dev, 1);
> +
> +	/* initialize glitch detect */
> +	__raw_writel(SRTC_LPPDR_INIT, ioaddr + SRTC_LPPDR);
> +	udelay(100);
> +
> +	/* clear lp interrupt status */
> +	__raw_writel(0xFFFFFFFF, ioaddr + SRTC_LPSR);
> +	udelay(100);
> +
> +	/* move out of init state */
> +	__raw_writel((SRTC_LPCR_IE | SRTC_LPCR_NSA), ioaddr + SRTC_LPCR);
> +
> +	udelay(100);
> +
> +	while ((__raw_readl(ioaddr + SRTC_LPSR) & SRTC_LPSR_IES) == 0)
> +		;
> +
> +	/* move out of non-valid state */
> +	__raw_writel((SRTC_LPCR_IE | SRTC_LPCR_NVE | SRTC_LPCR_NSA |
> +		      SRTC_LPCR_EN_LP), ioaddr + SRTC_LPCR);
> +
> +	udelay(100);

Where do all these udelay() come from? Are they really needed?

> +
> +	while ((__raw_readl(ioaddr + SRTC_LPSR) & SRTC_LPSR_NVES) == 0)
> +		;
> +
> +	__raw_writel(0xFFFFFFFF, ioaddr + SRTC_LPSR);
> +	udelay(100);
> +
> +	platform_set_drvdata(pdev, pdata);
> +	pdata->rtc =
> +	    devm_rtc_device_register(&pdev->dev, pdev->name, &mxc_rtc_ops,
> +				     THIS_MODULE);
> +	if (IS_ERR(pdata->rtc)) {
> +		ret = PTR_ERR(pdata->rtc);
> +		goto exit_put_clk;
> +	}
> +
> +	tv.tv_nsec = 0;
> +	tv.tv_sec = __raw_readl(ioaddr + SRTC_LPSCMR);

Why this? tv is set but unused.

> +
> +	/* By default, devices should wakeup if they can */
> +	/* So srtc is set as "should wakeup" as it can */
> +	device_init_wakeup(&pdev->dev, 1);

Why is this done twice, one time only when we have a valid irq? This
looks fishy.

> +
> +	return ret;
> +
> +exit_put_clk:
> +	clk_disable_unprepare(pdata->clk);
> +	return ret;
> +}
> +
> +static int __exit mxc_rtc_remove(struct platform_device *pdev)
> +{
> +	struct rtc_drv_data *pdata = platform_get_drvdata(pdev);
> +
> +	clk_disable_unprepare(pdata->clk);
> +	return 0;
> +}
> +
> +/*!
> + * This function is called to save the system time delta relative to
> + * the MXC RTC when enterring a low power state. This time delta is
> + * then used on resume to adjust the system time to account for time
> + * loss while suspended.
> + *
> + * @param   pdev  not used

Not true.

> + * @param   state Power state to enter.
> + *
> + * @return  The function always returns 0.
> + */
> +static int mxc_rtc_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	struct rtc_drv_data *pdata = platform_get_drvdata(pdev);
> +
> +	if (device_may_wakeup(&pdev->dev))
> +		enable_irq_wake(pdata->irq);
> +
> +	return 0;
> +}
> +
> +/*!
> + * This function is called to correct the system time based on the
> + * current MXC RTC time relative to the time delta saved during
> + * suspend.
> + *
> + * @param   pdev  not used

Not true.

> + *
> + * @return  The function always returns 0.
> + */
> +static int mxc_rtc_resume(struct platform_device *pdev)
> +{
> +	struct rtc_drv_data *pdata = platform_get_drvdata(pdev);
> +
> +	if (device_may_wakeup(&pdev->dev))
> +		disable_irq_wake(pdata->irq);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id mxc_ids[] = {
> +	{.compatible = "fsl,imx53-rtc",},
> +	{}
> +};
> +
> +/*!
> + * Contains pointers to the power management callback functions.

True, but doesn't contain any useful information.

> + */
> +static struct platform_driver mxc_rtc_driver = {
> +	.driver = {
> +		   .name = "mxc_rtc_v8",
> +		   .of_match_table = mxc_ids,
> +		   },

Indentation is broken here.

> +	.probe = mxc_rtc_probe,
> +	.remove = mxc_rtc_remove,
> +	.suspend = mxc_rtc_suspend,
> +	.resume = mxc_rtc_resume,
> +};
> +
> +module_platform_driver(mxc_rtc_driver);
> +
> +MODULE_AUTHOR("Freescale Semiconductor, Inc.");
> +MODULE_DESCRIPTION("Realtime Clock Driver (RTC)");
> +MODULE_LICENSE("GPL");

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v2 25/35] nds32: Build infrastructure
From: Greentime Hu @ 2017-11-30 10:01 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Geert Uytterhoeven, Greentime, Linux Kernel Mailing List,
	linux-arch, Thomas Gleixner, Jason Cooper, Marc Zyngier,
	Rob Herring, Networking, Vincent Chen, DTML, Al Viro,
	David Howells, Will Deacon, Daniel Lezcano,
	linux-serial@vger.kernel.org, Vincent Chen
In-Reply-To: <CAK8P3a3-wXAX+bRaDxs8+N5u6d-O6_Uwg7Wzp9m9=tSqQpPrEw@mail.gmail.com>

2017-11-30 17:30 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
> On Thu, Nov 30, 2017 at 6:48 AM, Greentime Hu <green.hu@gmail.com> wrote:
>> 2017-11-30 4:27 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
>>> On Wed, Nov 29, 2017 at 3:10 PM, Greentime Hu <green.hu@gmail.com> wrote:
>>>> 2017-11-29 19:57 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
>
>>> When you put them in a sorted list like I mentioned for simplicity, you
>>> could reduce the confusion by naming them differently, e.g.
>>> CONFIG_CPU_N10_OR_NEWER.
>>>
>>> Having only the CPU_CACHE_NONALIASING option is fine if you
>>> never need to make any other decisions based on the CPU core
>>> type, but then the help text should describe specifically which cases
>>> are affected (N10/N13/D13 with 4K page size), and you can decide to
>>> hide the option and make it always-on when using 8K page size.
>>>
>>>        Arnd
>>
>>
>> Hi, Arnd:
>>
>> I think I can use this name "CPU_V3" for all nds32 v3 compatible cpu.
>> It will be implemented like this.
>
> I think I'm still a bit confused about the relation between CPU cores
> and architecture levels. Is it correct to say that there are orthogonal,
> and that you can have e.g. an N10 core implementing either nds32v2
> or nds32v3?
>

Yup, we did having N10 cores are implementing either nds32v3 or
nds32v2, but nds32v2 are not used anymore.
We can assume every nds32 cores are v3.

> There is nothing wrong with that of course, it's just not what I
> expected from having worked with other architectures.
>
> I also see that GCC has no pipeline specific optimizations for
> specific cores, it just understands the differences between the
> architecture levels, so at least today there is way to pass e.g.
> "-march=nds32v2 -mtune=d15" to generate code that would
> work on both v2 and v3 but be optimized for d15.

Thanks. We will work on that.

>> config HWZOL
>>         bool "hardware zero overhead loop support"
>>         depends on CPU_D10 || CPU_D15
>>         default n
>>         help
>>           A set of Zero-Overhead Loop mechanism is provided to reduce the
>>           instruction fetch and execution overhead of loop-control instructions.
>>           It will save 3 registers($LB, $LC, $LE) for context saving if say Y.
>>           You don't need to save these registers if you can make sure your user
>>           program doesn't use these registers.
>>
>>           If unsure, say N.
>>
>> config CPU_CACHE_NONALIASING
>>         bool "Non-aliasing cache"
>>         depends on !CPU_N10 && !CPU_D10
>>         default n
>>         help
>>           If this CPU is using VIPT data cache and its cache way size is larger
>>           than page size, say N. If it is using PIPT data cache, say Y.
>>
>>           If unsure, say N.
>
> This looks ok, yes, but as Geert said, it would seem more intuitive to
> write it as
>
> config CPU_CACHE_ALIASING
>          bool "Aliasing VIPT cache"
>          depends on CPU_N10 || CPU_D10
>
>> choice
>>         prompt "CPU type"
>>         default CPU_V3
>> config CPU_N15
>>         bool "AndesCore N15"
>>         select CPU_CACHE_NONALIASING
>> config CPU_N13
>>         bool "AndesCore N13"
>>         select CPU_CACHE_NONALIASING if ANDES_PAGE_SIZE_8KB
>> config CPU_N10
>>         bool "AndesCore N10"
>> config CPU_D15
>>         bool "AndesCore D15"
>>         select CPU_CACHE_NONALIASING
>> config CPU_D10
>>         bool "AndesCore D10"
>> config CPU_V3
>>         bool "AndesCore v3 compatible"
>>         select ANDES_PAGE_SIZE_4KB
>> endchoice
>
> Two points here:
>
> - Generally you should not mix 'select' and 'depends on' like this.
>   Either you make the cache aliasing a user visible option that
>   uses 'depends on' with a combination of CPU cores, or you
>   make it a hidden option (with no string after the "bool" keyword)
>   that always gets selected from the per-cpu options.
>
> - There is a  little-known trick with choice statements that allows
>   you to use 'tristate' instead of 'bool' in the choice. In that case,
>   you can enable multiple options together as long as all of them
>   are 'm'.
>
>          Arnd


Thanks.
CPU_CACHE_ALIASING is more intuitive. I will apply it.

^ permalink raw reply

* Re: [PATCH 3/5] PCI: cadence: Add host driver for Cadence PCIe controller
From: Lorenzo Pieralisi @ 2017-11-30 10:06 UTC (permalink / raw)
  To: Cyrille Pitchen
  Cc: bhelgaas, kishon, linux-pci, adouglas, stelford, dgary, kgopi,
	eandrews, thomas.petazzoni, sureshp, nsekhar, linux-kernel, robh,
	devicetree
In-Reply-To: <20171129182514.GA1087@red-moon>

On Wed, Nov 29, 2017 at 06:25:15PM +0000, Lorenzo Pieralisi wrote:

[...]

> static void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int devfn,
> 				      int where)
> 
> > +cdns_pci_map_bus(struct pci_bus *bus, unsigned int devfn, int where)
> > +{
> > +	struct pci_host_bridge *bridge = pci_find_host_bridge(bus);
> > +	struct cdns_pcie_rc *rc = pci_host_bridge_priv(bridge);
> > +	struct cdns_pcie *pcie = &rc->pcie;
> > +	unsigned int busn = bus->number;
> > +	u32 addr0, desc0;
> > +
> > +	if (busn < rc->bus_range->start || busn > rc->bus_range->end)
> > +		return NULL;
> 
> It does not hurt but I wonder whether you really need this check.
> 
> > +	if (busn == rc->bus_range->start) {
> > +		if (devfn)
> 
> I suspect I know why you need this check but I ask you to explain it
> anyway if you do not mind please.
> 
> > +			return NULL;
> > +
> > +		return pcie->reg_base + (where & 0xfff);
> > +	}
> > +
> > +	/* Update Output registers for AXI region 0. */
> > +	addr0 = CDNS_PCIE_AT_OB_REGION_PCI_ADDR0_NBITS(12) |
> 
> Ok, so for every config access you reprogram addr0 to reflect the
> correct bus|devfn ID in the PCI bus TLP corresponding to an ECAM address
> in CPU physical address space, is my understanding correct ?

By re-reading it, it looks like this mechanism is there to just
associate a different RID TLP on the PCI bus to a fixed window
in CPU virtual address space.

> > +		CDNS_PCIE_AT_OB_REGION_PCI_ADDR0_DEVFN(devfn) |
> > +		CDNS_PCIE_AT_OB_REGION_PCI_ADDR0_BUS(busn);
> > +	cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_PCI_ADDR0(0), addr0);
> > +
> > +	/* Configuration Type 0 or Type 1 access. */
> > +	desc0 = CDNS_PCIE_AT_OB_REGION_DESC0_HARDCODED_RID |
> > +		CDNS_PCIE_AT_OB_REGION_DESC0_DEVFN(0);
> > +	/*
> > +	 * The bus number was already set once for all in desc1 by
> > +	 * cdns_pcie_host_init_address_translation().
> > +	 */
> > +	if (busn == rc->bus_range->start + 1)
> > +		desc0 |= CDNS_PCIE_AT_OB_REGION_DESC0_TYPE_CONF_TYPE0;
> > +	else
> > +		desc0 |= CDNS_PCIE_AT_OB_REGION_DESC0_TYPE_CONF_TYPE1;
> 
> I would like to ask you why you have to do it here and the root port
> does not figure it out by itself, I do not have the datasheet so I am
> just asking for my own information.
> 
> > +	cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_DESC0(0), desc0);
> > +
> > +	return rc->cfg_base + (where & 0xfff);
> > +}

[...]

> > +static int cdns_pcie_host_init(struct device *dev,
> > +			       struct list_head *resources,
> > +			       struct cdns_pcie_rc *rc)
> > +{
> > +	struct resource *bus_range = NULL;
> > +	int err;
> > +
> > +	/* Parse our PCI ranges and request their resources */
> > +	err = cdns_pcie_parse_request_of_pci_ranges(dev, resources, &bus_range);
> > +	if (err)
> > +		goto err_out;
> 
> I think that the err_out path should be part of:
> 
> cdns_pcie_parse_request_of_pci_ranges()
> 
> implementation and here you would just return.

I take it back, what you are doing is cleaner and allows you to
have code freeing the resource list in one single place so you
can leave this as-is.

Thanks,
Lorenzo

^ permalink raw reply

* Re: [PATCH 1/2] arm64: dts: renesas: r8a77970: use CPG core clock macros
From: Simon Horman @ 2017-11-30 10:09 UTC (permalink / raw)
  To: Sergei Shtylyov
  Cc: Rob Herring, Catalin Marinas, Will Deacon, linux-renesas-soc,
	devicetree, Magnus Damm, Mark Rutland, linux-arm-kernel
In-Reply-To: <d904863e-e8c9-76ad-9d12-fd36ed68c3b4@cogentembedded.com>

On Wed, Nov 29, 2017 at 08:00:52PM +0300, Sergei Shtylyov wrote:
> On 11/29/2017 12:55 PM, Simon Horman wrote:
> 
> > > Now that the commit ecadea00f588 ("dt-bindings: clock: Add R8A77970 CPG
> > > core clock definitions") has hit Linus' tree, we  can replace the bare
> > > numbers (we had to use to avoid a cross tree dependency) with these macro
> > > definitions...
> > > 
> > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> > 
> > Thanks, applied.
> 
>    Not seeing these 2 patches in the today's devel branch tho...

Strange, I don't see them either.
I have queued them up (again?).

^ permalink raw reply

* Re: [PATCH V2 5/9] PCI: Export pci_flags
From: Lorenzo Pieralisi @ 2017-11-30 10:24 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Manikanta Maddireddy, thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
	jonathanh-DDmLM1+adcrQT0dZR+AlfA, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	frowand.list-Re5JQEeQqe8AvxtiuMwx3w,
	bhelgaas-hpIqsD4AKlfQT0dZR+AlfA, rjw-LthD3rsA81gm4RdzfppkhA,
	tglx-hfZtesqFncYOwBW4kG4KsQ, vidyas-DDmLM1+adcrQT0dZR+AlfA,
	kthota-DDmLM1+adcrQT0dZR+AlfA, linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-pci-u79uwXL29TY76Z2rM5mHXA, linux-pm-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20171129170133.GC6469-1RhO1Y9PlrlHTL0Zs8A6p5iNqAH0jzoTYJqu5kTmcBRl57MIdRCFDg@public.gmane.org>

On Wed, Nov 29, 2017 at 11:01:33AM -0600, Bjorn Helgaas wrote:
> [+cc Lorenzo]
> 
> On Sun, Nov 26, 2017 at 01:02:09AM +0530, Manikanta Maddireddy wrote:
> > pci_flags variable is used in inline functions in 'pci.h', Tegra PCIe
> > driver use one of these functions pci_add_flags() and includes 'pci.h'.
> > Export pci_flags to allow Tegra PCIe host controller driver to be
> > compiled as loadable kernel module.
> 
> Here's the usage in tegra_pcie_probe():
> 
>   pci_add_flags(PCI_REASSIGN_ALL_RSRC | PCI_REASSIGN_ALL_BUS);
> 
> We've probably had this discussion before, but  I don't know why Tegra
> needs PCI_REASSIGN_ALL_RSRC and PCI_REASSIGN_ALL_BUS.
> 
> I would prefer to drop this usage of pci_add_flags() if possible.  It
> seems to be just an arm/powerpc thing and I'm not convinced it's
> really necessary.

It is hard to say if it is really necessary (because it depends
on firmware configuration - ie pci_scan_bridge()), that's the
problem.

I suspect it can trigger regressions if we do not set it (since
it affects what pcibios_assign_all_busses() returns on eg arm/arm64).

There are two things we can do:

1) Set it unconditionally in arch code (in a hook to be defined)
2) We remove it on a per-host bridge basis and ask for testing

I agree this may have trickled from host bridge to host bridge through
copy'n'paste and it is not based on any firmware assumtpion but I can't
say if it is really needed.

Lorenzo

^ permalink raw reply

* [PATCH v4] arm64: dts: renesas: r8a7795: Move nodes which have no reg property out of bus
From: Simon Horman @ 2017-11-30 10:25 UTC (permalink / raw)
  To: linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA
  Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Magnus Damm,
	Arnd Bergmann, Rob Herring, Geert Uytterhoeven,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Simon Horman

Move pmu_a5[73], timer and thermal-zones nodes from soc node to root node.
The nodes that have been moved do not have any register properties and thus
shouldn't be placed on the bus.

This problem is flagged by the compiler as follows:
$ make
...
  DTC     arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dtb
arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dtb: Warning (simple_bus_reg): Node /soc/pmu_a57 missing or empty reg/ranges property
arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dtb: Warning (simple_bus_reg): Node /soc/pmu_a53 missing or empty reg/ranges property
arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dtb: Warning (simple_bus_reg): Node /soc/timer missing or empty reg/ranges property
arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dtb: Warning (simple_bus_reg): Node /soc/thermal-zones missing or empty reg/ranges property
  DTC     arch/arm64/boot/dts/renesas/r8a7795-h3ulcb.dtb
arch/arm64/boot/dts/renesas/r8a7795-h3ulcb.dtb: Warning (simple_bus_reg): Node /soc/pmu_a57 missing or empty reg/ranges property
arch/arm64/boot/dts/renesas/r8a7795-h3ulcb.dtb: Warning (simple_bus_reg): Node /soc/pmu_a53 missing or empty reg/ranges property
arch/arm64/boot/dts/renesas/r8a7795-h3ulcb.dtb: Warning (simple_bus_reg): Node /soc/timer missing or empty reg/ranges property
arch/arm64/boot/dts/renesas/r8a7795-h3ulcb.dtb: Warning (simple_bus_reg): Node /soc/thermal-zones missing or empty reg/ranges property
  DTC     arch/arm64/boot/dts/renesas/r8a7795-h3ulcb-kf.dtb
arch/arm64/boot/dts/renesas/r8a7795-h3ulcb-kf.dtb: Warning (simple_bus_reg): Node /soc/pmu_a57 missing or empty reg/ranges property
arch/arm64/boot/dts/renesas/r8a7795-h3ulcb-kf.dtb: Warning (simple_bus_reg): Node /soc/pmu_a53 missing or empty reg/ranges property
arch/arm64/boot/dts/renesas/r8a7795-h3ulcb-kf.dtb: Warning (simple_bus_reg): Node /soc/timer missing or empty reg/ranges property
arch/arm64/boot/dts/renesas/r8a7795-h3ulcb-kf.dtb: Warning (simple_bus_reg): Node /soc/thermal-zones missing or empty reg/ranges property
  DTC     arch/arm64/boot/dts/renesas/r8a7795-salvator-xs.dtb
arch/arm64/boot/dts/renesas/r8a7795-salvator-xs.dtb: Warning (simple_bus_reg): Node /soc/pmu_a57 missing or empty reg/ranges property
arch/arm64/boot/dts/renesas/r8a7795-salvator-xs.dtb: Warning (simple_bus_reg): Node /soc/pmu_a53 missing or empty reg/ranges property
arch/arm64/boot/dts/renesas/r8a7795-salvator-xs.dtb: Warning (simple_bus_reg): Node /soc/timer missing or empty reg/ranges property
arch/arm64/boot/dts/renesas/r8a7795-salvator-xs.dtb: Warning (simple_bus_reg): Node /soc/thermal-zones missing or empty reg/ranges property
  DTC     arch/arm64/boot/dts/renesas/r8a7795-es1-salvator-x.dtb
arch/arm64/boot/dts/renesas/r8a7795-es1-salvator-x.dtb: Warning (simple_bus_reg): Node /soc/pmu_a57 missing or empty reg/ranges property
arch/arm64/boot/dts/renesas/r8a7795-es1-salvator-x.dtb: Warning (simple_bus_reg): Node /soc/pmu_a53 missing or empty reg/ranges property
arch/arm64/boot/dts/renesas/r8a7795-es1-salvator-x.dtb: Warning (simple_bus_reg): Node /soc/timer missing or empty reg/ranges property
arch/arm64/boot/dts/renesas/r8a7795-es1-salvator-x.dtb: Warning (simple_bus_reg): Node /soc/thermal-zones missing or empty reg/ranges property
  DTC     arch/arm64/boot/dts/renesas/r8a7795-es1-h3ulcb.dtb
arch/arm64/boot/dts/renesas/r8a7795-es1-h3ulcb.dtb: Warning (simple_bus_reg): Node /soc/pmu_a57 missing or empty reg/ranges property
arch/arm64/boot/dts/renesas/r8a7795-es1-h3ulcb.dtb: Warning (simple_bus_reg): Node /soc/pmu_a53 missing or empty reg/ranges property
arch/arm64/boot/dts/renesas/r8a7795-es1-h3ulcb.dtb: Warning (simple_bus_reg): Node /soc/timer missing or empty reg/ranges property
arch/arm64/boot/dts/renesas/r8a7795-es1-h3ulcb.dtb: Warning (simple_bus_reg): Node /soc/thermal-zones missing or empty reg/ranges property
  DTC     arch/arm64/boot/dts/renesas/r8a7795-es1-h3ulcb-kf.dtb
arch/arm64/boot/dts/renesas/r8a7795-es1-h3ulcb-kf.dtb: Warning (simple_bus_reg): Node /soc/pmu_a57 missing or empty reg/ranges property
arch/arm64/boot/dts/renesas/r8a7795-es1-h3ulcb-kf.dtb: Warning (simple_bus_reg): Node /soc/pmu_a53 missing or empty reg/ranges property
arch/arm64/boot/dts/renesas/r8a7795-es1-h3ulcb-kf.dtb: Warning (simple_bus_reg): Node /soc/timer missing or empty reg/ranges property
arch/arm64/boot/dts/renesas/r8a7795-es1-h3ulcb-kf.dtb: Warning (simple_bus_reg): Node /soc/thermal-zones missing or empty reg/ranges property

Signed-off-by: Simon Horman <horms+renesas-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
---
v4
* Use interrupts-extended rather than interrupt-parent + interrupts
  in /pmu_* and /timer nodes as seems to be preferred.
v3
* Add interrupt-parent property to /pmu_* and /timer nodes as
  it is no longer inherited from the parent node
v2
* Preserve alphabetical order of nodes present in root node.

Based on renesas-devel-20171130-v4.15-rc1
---
 arch/arm64/boot/dts/renesas/r8a7795.dtsi | 138 ++++++++++++++++---------------
 1 file changed, 71 insertions(+), 67 deletions(-)

diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
index 6db4f10376a1..a851c88e1e04 100644
--- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
@@ -184,6 +184,30 @@
 		clock-frequency = <0>;
 	};
 
+	pmu_a57 {
+		compatible = "arm,cortex-a57-pmu";
+		interrupts-extended = <&gic GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>,
+		                      <&gic GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>,
+				      <&gic GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>,
+				      <&gic GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-affinity = <&a57_0>,
+				     <&a57_1>,
+				     <&a57_2>,
+				     <&a57_3>;
+	};
+
+	pmu_a53 {
+		compatible = "arm,cortex-a53-pmu";
+		interrupts-extended = <&gic GIC_SPI 84 IRQ_TYPE_LEVEL_HIGH>,
+				      <&gic GIC_SPI 85 IRQ_TYPE_LEVEL_HIGH>,
+				      <&gic GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>,
+				      <&gic GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-affinity = <&a53_0>,
+				     <&a53_1>,
+				     <&a53_2>,
+				     <&a53_3>;
+	};
+
 	soc: soc {
 		compatible = "simple-bus";
 		interrupt-parent = <&gic>;
@@ -338,42 +362,6 @@
 			resets = <&cpg 905>;
 		};
 
-		pmu_a57 {
-			compatible = "arm,cortex-a57-pmu";
-			interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>;
-			interrupt-affinity = <&a57_0>,
-					     <&a57_1>,
-					     <&a57_2>,
-					     <&a57_3>;
-		};
-
-		pmu_a53 {
-			compatible = "arm,cortex-a53-pmu";
-			interrupts = <GIC_SPI 84 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 85 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>;
-			interrupt-affinity = <&a53_0>,
-					     <&a53_1>,
-					     <&a53_2>,
-					     <&a53_3>;
-		};
-
-		timer {
-			compatible = "arm,armv8-timer";
-			interrupts = <GIC_PPI 13
-					(GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
-				     <GIC_PPI 14
-					(GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
-				     <GIC_PPI 11
-					(GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
-				     <GIC_PPI 10
-					(GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>;
-		};
-
 		cpg: clock-controller@e6150000 {
 			compatible = "renesas,r8a7795-cpg-mssr";
 			reg = <0 0xe6150000 0 0x1000>;
@@ -2331,47 +2319,63 @@
 			#thermal-sensor-cells = <1>;
 			status = "okay";
 		};
+	};
 
-		thermal-zones {
-			sensor_thermal1: sensor-thermal1 {
-				polling-delay-passive = <250>;
-				polling-delay = <1000>;
-				thermal-sensors = <&tsc 0>;
+	timer {
+		compatible = "arm,armv8-timer";
+		interrupts-extended = <&gic GIC_PPI 13
+				       (GIC_CPU_MASK_SIMPLE(8) |
+				       IRQ_TYPE_LEVEL_LOW)>,
+				      <&gic GIC_PPI 14
+				       (GIC_CPU_MASK_SIMPLE(8) |
+				       IRQ_TYPE_LEVEL_LOW)>,
+				      <&gic GIC_PPI 11
+				       (GIC_CPU_MASK_SIMPLE(8) |
+				       IRQ_TYPE_LEVEL_LOW)>,
+				      <&gic GIC_PPI 10
+				       (GIC_CPU_MASK_SIMPLE(8) |
+				       IRQ_TYPE_LEVEL_LOW)>;
+	};
 
-				trips {
-					sensor1_crit: sensor1-crit {
-						temperature = <120000>;
-						hysteresis = <2000>;
-						type = "critical";
-					};
+	thermal-zones {
+		sensor_thermal1: sensor-thermal1 {
+			polling-delay-passive = <250>;
+			polling-delay = <1000>;
+			thermal-sensors = <&tsc 0>;
+
+			trips {
+				sensor1_crit: sensor1-crit {
+					temperature = <120000>;
+					hysteresis = <2000>;
+					type = "critical";
 				};
 			};
+		};
 
-			sensor_thermal2: sensor-thermal2 {
-				polling-delay-passive = <250>;
-				polling-delay = <1000>;
-				thermal-sensors = <&tsc 1>;
+		sensor_thermal2: sensor-thermal2 {
+			polling-delay-passive = <250>;
+			polling-delay = <1000>;
+			thermal-sensors = <&tsc 1>;
 
-				trips {
-					sensor2_crit: sensor2-crit {
-						temperature = <120000>;
-						hysteresis = <2000>;
-						type = "critical";
-					};
+			trips {
+				sensor2_crit: sensor2-crit {
+					temperature = <120000>;
+					hysteresis = <2000>;
+					type = "critical";
 				};
 			};
+		};
 
-			sensor_thermal3: sensor-thermal3 {
-				polling-delay-passive = <250>;
-				polling-delay = <1000>;
-				thermal-sensors = <&tsc 2>;
+		sensor_thermal3: sensor-thermal3 {
+			polling-delay-passive = <250>;
+			polling-delay = <1000>;
+			thermal-sensors = <&tsc 2>;
 
-				trips {
-					sensor3_crit: sensor3-crit {
-						temperature = <120000>;
-						hysteresis = <2000>;
-						type = "critical";
-					};
+			trips {
+				sensor3_crit: sensor3-crit {
+					temperature = <120000>;
+					hysteresis = <2000>;
+					type = "critical";
 				};
 			};
 		};
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related

* Re: [PATCH v5 01/16] dt-bindings: mtd: gpmc-onenand: Update properties description
From: Boris Brezillon @ 2017-11-30 10:25 UTC (permalink / raw)
  To: Ladislav Michl
  Cc: linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-omap-u79uwXL29TY76Z2rM5mHXA, Roger Quadros, Peter Ujfalusi,
	Kyungmin Park, Aaro Koskinen, Tony Lindgren, Rob Herring,
	devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20171115162458.rtompijrsaobdemk@lenoch>

+Rob and the DT ML

On Wed, 15 Nov 2017 17:24:58 +0100
Ladislav Michl <ladis-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org> wrote:

> Compatible property is required for OMAP2+ mtd driver. Also
> add INT pin gpio description and delete unused dma-channel
> property.
> 
> Signed-off-by: Ladislav Michl <ladis-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org>
> Reviewed-by: Peter Ujfalusi <peter.ujfalusi-l0cyMroinI0@public.gmane.org>
> Acked-by: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>
> ---
> 
> Changes in v5:
> - renamed R/B pin to INT pin
> 
> Changes in v4:
> - new patch
> 
> Changes in v3: None
> Changes in v2: None
> 
>  Documentation/devicetree/bindings/mtd/gpmc-onenand.txt | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/gpmc-onenand.txt b/Documentation/devicetree/bindings/mtd/gpmc-onenand.txt
> index b6e8bfd024f4..e9f01a963a0a 100644
> --- a/Documentation/devicetree/bindings/mtd/gpmc-onenand.txt
> +++ b/Documentation/devicetree/bindings/mtd/gpmc-onenand.txt
> @@ -9,13 +9,14 @@ Documentation/devicetree/bindings/memory-controllers/omap-gpmc.txt
>  
>  Required properties:
>  
> + - compatible:		"ti,omap2-onenand"

Don't you break backward compat by adding this new requirement?

>   - reg:			The CS line the peripheral is connected to
> - - gpmc,device-width	Width of the ONENAND device connected to the GPMC
> + - gpmc,device-width:	Width of the ONENAND device connected to the GPMC
>  			in bytes. Must be 1 or 2.
>  
>  Optional properties:
>  
> - - dma-channel:		DMA Channel index
> + - int-gpios:		GPIO specifier for the INT pin.
>  
>  For inline partition table parsing (optional):
>  
> @@ -35,6 +36,7 @@ Example for an OMAP3430 board:
>  		#size-cells = <1>;
>  
>  		onenand@0 {
> +			compatible = "ti,omap2-onenand";
>  			reg = <0 0 0>; /* CS0, offset 0 */
>  			gpmc,device-width = <2>;
>  

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v5] mfd: syscon: Add hardware spinlock support
From: Vladimir Zapolskiy @ 2017-11-30 10:38 UTC (permalink / raw)
  To: Baolin Wang, lee.jones, arnd, robh+dt, mark.rutland
  Cc: broonie, linux-kernel, devicetree
In-Reply-To: <9bd0bfdaf4e869e6216a0ef032b52cd28b33ab16.1512032962.git.baolin.wang@linaro.org>

On 11/30/2017 11:10 AM, Baolin Wang wrote:
> Some system control registers need hardware spinlock to synchronize
> between the multiple subsystems, so we should add hardware spinlock
> support for syscon.
> 
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
> Changes since v4:
>  - Add one exapmle to show how to add hwlock.
>  - Fix the coding style issue.
> 
> Changes since v3:
>  - Add error handling for of_hwspin_lock_get_id()
> 
> Changes since v2:
>  - Add acked tag from Rob.
> 
> Changes since v1:
>  - Remove timeout configuration.
>  - Modify the binding file to add hwlocks.
> ---
>  Documentation/devicetree/bindings/mfd/syscon.txt |    8 ++++++++
>  drivers/mfd/syscon.c                             |   21 +++++++++++++++++++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/syscon.txt b/Documentation/devicetree/bindings/mfd/syscon.txt
> index 8b92d45..25d9e9c 100644
> --- a/Documentation/devicetree/bindings/mfd/syscon.txt
> +++ b/Documentation/devicetree/bindings/mfd/syscon.txt
> @@ -16,9 +16,17 @@ Required properties:
>  Optional property:
>  - reg-io-width: the size (in bytes) of the IO accesses that should be
>    performed on the device.
> +- hwlocks: reference to a phandle of a hardware spinlock provider node.
>  
>  Examples:
>  gpr: iomuxc-gpr@20e0000 {
>  	compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
>  	reg = <0x020e0000 0x38>;
> +	hwlocks = <&hwlock1 1>;
> +};
> +
> +hwlock1: hwspinlock@40500000 {
> +	...
> +	reg = <0x40500000 0x1000>;
> +	#hwlock-cells = <1>;
>  };
> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> index b93fe4c..5b67dbc 100644
> --- a/drivers/mfd/syscon.c
> +++ b/drivers/mfd/syscon.c
> @@ -13,6 +13,7 @@
>   */
>  
>  #include <linux/err.h>
> +#include <linux/hwspinlock.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
>  #include <linux/list.h>
> @@ -87,6 +88,26 @@ static struct syscon *of_syscon_register(struct device_node *np)
>  	if (ret)
>  		reg_io_width = 4;
>  
> +	ret = of_hwspin_lock_get_id(np, 0);
> +	if (ret > 0) {
> +		syscon_config.hwlock_id = ret;
> +		syscon_config.hwlock_mode = HWLOCK_IRQSTATE;
> +	} else {
> +		switch (ret) {
> +		case -ENOENT:
> +			/* Ignore missing hwlock, it's optional. */
> +			break;
> +		case 0:
> +			ret = -EINVAL;
> +			/* fall-through */

It breaks the execution on all kernels without CONFIG_HWSPINLOCK enabled.

> +		default:
> +			pr_err("Failed to retrieve valid hwlock: %d\n", ret);
> +			/* fall-through */
> +		case -EPROBE_DEFER:
> +			goto err_regmap;
> +		}
> +	}
> +
>  	syscon_config.reg_stride = reg_io_width;
>  	syscon_config.val_bits = reg_io_width * 8;
>  	syscon_config.max_register = resource_size(&res) - reg_io_width;
> 

--
With best wishes,
Vladimir

^ permalink raw reply

* Re: [PATCH v5] mfd: syscon: Add hardware spinlock support
From: Baolin Wang @ 2017-11-30 10:45 UTC (permalink / raw)
  To: Vladimir Zapolskiy
  Cc: Lee Jones, Arnd Bergmann, Rob Herring, Mark Rutland, Mark Brown,
	LKML, DTML
In-Reply-To: <78c06786-8bfc-3f15-fdb5-061691010509@mentor.com>

On 30 November 2017 at 18:38, Vladimir Zapolskiy
<vladimir_zapolskiy@mentor.com> wrote:
> On 11/30/2017 11:10 AM, Baolin Wang wrote:
>> Some system control registers need hardware spinlock to synchronize
>> between the multiple subsystems, so we should add hardware spinlock
>> support for syscon.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> Acked-by: Rob Herring <robh@kernel.org>
>> ---
>> Changes since v4:
>>  - Add one exapmle to show how to add hwlock.
>>  - Fix the coding style issue.
>>
>> Changes since v3:
>>  - Add error handling for of_hwspin_lock_get_id()
>>
>> Changes since v2:
>>  - Add acked tag from Rob.
>>
>> Changes since v1:
>>  - Remove timeout configuration.
>>  - Modify the binding file to add hwlocks.
>> ---
>>  Documentation/devicetree/bindings/mfd/syscon.txt |    8 ++++++++
>>  drivers/mfd/syscon.c                             |   21 +++++++++++++++++++++
>>  2 files changed, 29 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/syscon.txt b/Documentation/devicetree/bindings/mfd/syscon.txt
>> index 8b92d45..25d9e9c 100644
>> --- a/Documentation/devicetree/bindings/mfd/syscon.txt
>> +++ b/Documentation/devicetree/bindings/mfd/syscon.txt
>> @@ -16,9 +16,17 @@ Required properties:
>>  Optional property:
>>  - reg-io-width: the size (in bytes) of the IO accesses that should be
>>    performed on the device.
>> +- hwlocks: reference to a phandle of a hardware spinlock provider node.
>>
>>  Examples:
>>  gpr: iomuxc-gpr@20e0000 {
>>       compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
>>       reg = <0x020e0000 0x38>;
>> +     hwlocks = <&hwlock1 1>;
>> +};
>> +
>> +hwlock1: hwspinlock@40500000 {
>> +     ...
>> +     reg = <0x40500000 0x1000>;
>> +     #hwlock-cells = <1>;
>>  };
>> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
>> index b93fe4c..5b67dbc 100644
>> --- a/drivers/mfd/syscon.c
>> +++ b/drivers/mfd/syscon.c
>> @@ -13,6 +13,7 @@
>>   */
>>
>>  #include <linux/err.h>
>> +#include <linux/hwspinlock.h>
>>  #include <linux/io.h>
>>  #include <linux/module.h>
>>  #include <linux/list.h>
>> @@ -87,6 +88,26 @@ static struct syscon *of_syscon_register(struct device_node *np)
>>       if (ret)
>>               reg_io_width = 4;
>>
>> +     ret = of_hwspin_lock_get_id(np, 0);
>> +     if (ret > 0) {
>> +             syscon_config.hwlock_id = ret;
>> +             syscon_config.hwlock_mode = HWLOCK_IRQSTATE;
>> +     } else {
>> +             switch (ret) {
>> +             case -ENOENT:
>> +                     /* Ignore missing hwlock, it's optional. */
>> +                     break;
>> +             case 0:
>> +                     ret = -EINVAL;
>> +                     /* fall-through */
>
> It breaks the execution on all kernels without CONFIG_HWSPINLOCK enabled.

Ah, your are correct. Sorry, I missed this case. I will fix this in
next version. Thanks.

-- 
Baolin.wang
Best Regards

^ permalink raw reply

* RE: [patch v12 2/4] drivers: jtag: Add Aspeed SoC 24xx and 25xx families JTAG master driver
From: Oleksandr Shamray @ 2017-11-30 10:49 UTC (permalink / raw)
  To: Philippe Ombredanne, Kun Yi
  Cc: Greg Kroah-Hartman, arnd@arndb.de, system-sw-low-level,
	devicetree@vger.kernel.org, jiri@resnulli.us, Vadim Pasternak,
	linux-api@vger.kernel.org, OpenBMC Maillist, LKML,
	openocd-devel-owner@lists.sourceforge.net, mec@shout.net,
	Jiri Pirko, robh+dt@kernel.org, linux-serial@vger.kernel.org,
	tklauser@distanz.ch, Mauro Carvalho Chehab, Dav
In-Reply-To: <CAOFm3uHg8n4A8opaOC3=pNtM1yJU6iy5Ta8XtGOvqT-Euv=X1w@mail.gmail.com>

Thnaks for point 

Best Regards,
Oleksandr Shamray

> -----Original Message-----
> From: Philippe Ombredanne [mailto:pombredanne@nexb.com]
> Sent: Thursday, November 30, 2017 10:21 AM
> To: Kun Yi <kunyi@google.com>
> Cc: Oleksandr Shamray <oleksandrs@mellanox.com>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; arnd@arndb.de; system-sw-low-level <system-
> sw-low-level@mellanox.com>; devicetree@vger.kernel.org; jiri@resnulli.us;
> Vadim Pasternak <vadimp@mellanox.com>; linux-api@vger.kernel.org;
> OpenBMC Maillist <openbmc@lists.ozlabs.org>; LKML <linux-
> kernel@vger.kernel.org>; openocd-devel-owner@lists.sourceforge.net;
> mec@shout.net; Jiri Pirko <jiri@mellanox.com>; robh+dt@kernel.org; linux-
> serial@vger.kernel.org; tklauser@distanz.ch; Mauro Carvalho Chehab
> <mchehab@kernel.org>; David S. Miller <davem@davemloft.net>; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [patch v12 2/4] drivers: jtag: Add Aspeed SoC 24xx and 25xx
> families JTAG master driver
> 
> On Wed, Nov 29, 2017 at 11:51 PM, Kun Yi <kunyi@google.com> wrote:
> > On Tue, Nov 14, 2017 at 8:11 AM, Oleksandr Shamray
> > <oleksandrs@mellanox.com> wrote:
> []
> >> diff --git a/drivers/jtag/jtag-aspeed.c b/drivers/jtag/jtag-aspeed.c
> >> new file mode 100644 index 0000000..a6e2417
> >> --- /dev/null
> >> +++ b/drivers/jtag/jtag-aspeed.c
> >> @@ -0,0 +1,782 @@
> >> +/*
> >> + * drivers/jtag/aspeed-jtag.c
> >> + *
> >> + * Copyright (c) 2017 Mellanox Technologies. All rights reserved.
> >> + * Copyright (c) 2017 Oleksandr Shamray <oleksandrs@mellanox.com>
> >> + *
> >> + * Released under the GPLv2 only.
> >> + * SPDX-License-Identifier: GPL-2.0
> >> + */
> 
> I think the SPDX id should be on the first line as requested by Linus and
> documented by Thomas (tglx) and Greg (greg-kh). And it should use //
> comments, e.g:
> 
> // SPDX-License-Identifier: GPL-2.0
> 
> See the threads discussing all these.
> 
> 
> --
> Cordially
> Philippe Ombredanne

^ permalink raw reply

* Re: [PATCH] rtc: add mxc driver for i.MX53
From: Lothar Waßmann @ 2017-11-30 10:53 UTC (permalink / raw)
  To: Sascha Hauer
  Cc: linux-kernel-dev-QonKdJ6Bx35Wk0Htik3J/w, Mark Rutland,
	Alessandro Zummo, Patrick Bruenn,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, Rob Herring, Russell King, Noel Vellemans,
	Alexandre Belloni, Sascha Hauer, Fabio Estevam, Shawn Guo,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	open list:REAL TIME CLOCK (RTC) SUBSYSTEM
In-Reply-To: <20171130095047.4sl7rfa5cc6rtmig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

Hi,

On Thu, 30 Nov 2017 10:50:47 +0100 Sascha Hauer wrote:
> On Tue, Nov 28, 2017 at 08:39:27AM +0100, linux-kernel-dev-QonKdJ6Bx35Wk0Htik3J/w@public.gmane.org wrote:
[...]
> > +/*! MXC RTC Power management control */
> > +static int mxc_rtc_probe(struct platform_device *pdev)
> > +{
> > +	struct timespec tv;
> > +	struct resource *res;
> > +	struct rtc_drv_data *pdata = NULL;
> 
> No need to initialize.
> 
> > +	void __iomem *ioaddr;
> > +	int ret = 0;
>
Same here.


Lothar Waßmann
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v3 0/4] Fixes for omapdrm on OpenPandora and GTA04
From: Tomi Valkeinen @ 2017-11-30 10:54 UTC (permalink / raw)
  To: H. Nikolaus Schaller, Thierry Reding, David Airlie, Rob Herring,
	Mark Rutland, Benoît Cousson, Tony Lindgren, Russell King,
	Bartlomiej Zolnierkiewicz, Laurent Pinchart, Julia Lawall,
	Sean Paul
  Cc: devicetree, linux-fbdev, letux-kernel, linux-kernel, dri-devel,
	kernel, linux-omap, linux-arm-kernel
In-Reply-To: <cover.1511884135.git.hns@goldelico.com>

On 28/11/17 17:48, H. Nikolaus Schaller wrote:
> Changes V3:
> * stay compatible with old DTB files which still use "toppoly" (suggested by Tomi Valkeinen)
> * replaced MODULE_ALIAS entries by MODULE_DEVICE_TABLE (suggested by Andrew F. Davis)
> * removed DSI VDDS patch as it has already been accepted
> 
> 2017-11-16 09:50:22: Changes V2:
> * replaced patch to fix DSI VDDS for OMAP3 by equivalent patch from Laurent Pinchart
> * keep previous compatibility option in panel driver to handle older device tree binaries
> 
> 2017-11-08 22:09:36:
> This patch set fixes vendor names of the panels
> and fixes a problem on omapdrm with enabling
> VDD_DSI for OMAP3 which is needed for displaying
> the Red and Green channel on OMAP3530 (Pandora).
> 
> H. Nikolaus Schaller (4):
>   omapdrm: panel: fix compatible vendor string for td028ttec1
>   omapdrm: panel: td028ttec1: replace MODULE_ALIAS by
>     MODULE_DEVICE_TABLE
>   DTS: GTA04: fix panel compatibility string
>   DTS: Pandora: fix panel compatibility string
> 
>  .../panel/{toppoly,td028ttec1.txt => tpo,td028ttec1.txt}    |  4 ++--
>  arch/arm/boot/dts/omap3-gta04.dtsi                          |  2 +-
>  arch/arm/boot/dts/omap3-pandora-common.dtsi                 |  2 +-
>  drivers/gpu/drm/omapdrm/displays/panel-tpo-td028ttec1.c     | 13 ++++++++++++-
>  .../fbdev/omap2/omapfb/displays/panel-tpo-td028ttec1.c      | 12 +++++++++++-
>  5 files changed, 27 insertions(+), 6 deletions(-)
>  rename Documentation/devicetree/bindings/display/panel/{toppoly,td028ttec1.txt => tpo,td028ttec1.txt} (84%)
> 

Thanks. I have picked up patches 1 and 2.

3 can be applied when 1 & 2 are in. The change in 4 could be applied
independently, but it conflicts with 3.

Tony, how do you want to handle 3 and 4? I will push 1 and 2 to v4.16. I
don't think they are real issues, so I don't see a reason to push them
as fixes to v4.15. I think they are mostly just cleanups, and we might
as well wait until v4.17, but that's quite far away...

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply

* Re: [PATCH v2 27/35] irqchip: Andestech Internal Vector Interrupt Controller driver
From: Marc Zyngier @ 2017-11-30 10:57 UTC (permalink / raw)
  To: Greentime Hu
  Cc: Greentime, Linux Kernel Mailing List, Arnd Bergmann, linux-arch,
	Thomas Gleixner, Jason Cooper, Rob Herring, netdev, Vincent Chen,
	DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
	linux-serial, Rick Chen
In-Reply-To: <CAEbi=3eXP1cfNDhu9YjZbjPghgxm9xmEtjEBO=KVOnWZyPqVbw@mail.gmail.com>

On Wed, Nov 29 2017 at 11:23:34 pm GMT, Greentime Hu <green.hu@gmail.com> wrote:

Hi Greentime,

>>> +}
>>> +
>>> +static void ativic32_mask_ack_irq(struct irq_data *data)
>>> +{
>>> +     unsigned long int_mask2 = __nds32__mfsr(NDS32_SR_INT_MASK2);
>>> + __nds32__mtsr_dsb(int_mask2 & (~(1 << data->hwirq)),
>>> NDS32_SR_INT_MASK2);
>>> +     __nds32__mtsr_dsb((1 << data->hwirq), NDS32_SR_INT_PEND2);
>>
>> This is effectively MASK+ACK, so you're better off just writing it as
>> such. And since there is no advantage in your implementation in having
>> MASK_ACK over MASK+ACK, I suggest you remove this function completely,
>> and rely on the core code which will call them in sequence.
>
> I think mask_ack is still better than mask + ack because we don't need
> to do two function call.
> We can save a prologue and a epilogue. It will benefit interrupt latency.

Can you actually measure this? Your CPU would have to be extremely slow
if you could see the impact of an extra function call on interrupt
latency. From a maintenance perspective, this isn't very nice (it is
effectively code duplication).

I suggest you start with the simplest version of the code, and provide
an optimisation once you can measurably demonstrate that mask_ack is
better.

[...]

>>> + irq_set_chip_and_handler(virq, &ativic32_chip, handle_edge_irq);
>>> +     else
>>> + irq_set_chip_and_handler(virq, &ativic32_chip, handle_level_irq);
>>
>> Since you do not express the trigger in DT, you need to tell the core
>> about it by calling irqd_set_trigger_type() with the right setting.
>>
>
> Since the comments say so, I will add ativic32_set_type() for irq_set_type()
> in the next version patch.
>
> /*
>  * Must only be called inside irq_chip.irq_set_type() functions.
>  */
> static inline void irqd_set_trigger_type(struct irq_data *d, u32 type)
> {
>         __irqd_to_state(d) &= ~IRQD_TRIGGER_MASK;
>         __irqd_to_state(d) |= type & IRQD_TRIGGER_MASK;
> }
>
> It will be like this.
> static int ativic32_set_type(struct irq_data *data, unsigned int flow_type)
> {
>         irqd_set_trigger_type(data, flow_type);
>         return IRQ_SET_MASK_OK;
> }

This feels wrong. Your interrupt controller doesn't seem to support the
trigger being changed, so irq_set_type would be a bit pointless. A
driver trying to set a level trigger on an edge interrupt would succeed,
and that is as bad as it gets. All you need is to expose the capability
of the HW when you register the flow handler instead of adding a rather
misleading callback.

Thanks,

	M.
-- 
Jazz is not dead, it just smell funny.

^ permalink raw reply

* Re: [PATCH] rtc: add mxc driver for i.MX53
From: Lothar Waßmann @ 2017-11-30 11:21 UTC (permalink / raw)
  To: linux-kernel-dev-QonKdJ6Bx35Wk0Htik3J/w
  Cc: Alessandro Zummo, Alexandre Belloni, Mark Rutland,
	open list:REAL TIME CLOCK (RTC) SUBSYSTEM, Patrick Bruenn,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, Russell King, Noel Vellemans, Rob Herring,
	Sascha Hauer, Fabio Estevam, Shawn Guo,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <20171128073927.12035-1-linux-kernel-dev-QonKdJ6Bx35Wk0Htik3J/w@public.gmane.org>

Hi,

On Tue, 28 Nov 2017 08:39:27 +0100 linux-kernel-dev-QonKdJ6Bx35Wk0Htik3J/w@public.gmane.org wrote:
> From: Patrick Bruenn <p.bruenn-QonKdJ6Bx35Wk0Htik3J/w@public.gmane.org>
[...]
> +/*!
> + * This function is the RTC interrupt service routine.
> + *
> + * @param  irq          RTC IRQ number
> + * @param  dev_id       device ID which is not used
> + *
> + * @return IRQ_HANDLED as defined in the include/linux/interrupt.h file.
> + */
> +static irqreturn_t mxc_rtc_interrupt(int irq, void *dev_id)
> +{
> +	struct platform_device *pdev = dev_id;
> +	struct rtc_drv_data *pdata = platform_get_drvdata(pdev);
> +	void __iomem *ioaddr = pdata->ioaddr;
> +	u32 lp_status, lp_cr;
> +	u32 events = 0;
> +
> +	lp_status = __raw_readl(ioaddr + SRTC_LPSR);
> +	lp_cr = __raw_readl(ioaddr + SRTC_LPCR);
> +
> +	/* update irq data & counter */
> +	if (lp_status & SRTC_LPSR_ALP) {
> +		if (lp_cr & SRTC_LPCR_ALP)
> +			events |= (RTC_AF | RTC_IRQF);
> +
> +		/* disable further lp alarm interrupts */
> +		lp_cr &= ~(SRTC_LPCR_ALP | SRTC_LPCR_WAE);
> +	}
> +
> +	/* Update interrupt enables */
> +	__raw_writel(lp_cr, ioaddr + SRTC_LPCR);
> +
> +	/* clear interrupt status */
> +	__raw_writel(lp_status, ioaddr + SRTC_LPSR);
> +
> +	rtc_write_sync_lp(ioaddr);
> +	rtc_update_irq(pdata->rtc, 1, events);
> +	return IRQ_HANDLED;
> +}
> +
see comment below...

> +/*! MXC RTC Power management control */
> +static int mxc_rtc_probe(struct platform_device *pdev)
> +{
> +	struct timespec tv;
> +	struct resource *res;
> +	struct rtc_drv_data *pdata = NULL;
> +	void __iomem *ioaddr;
> +	int ret = 0;
> +
> +	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENODEV;
> +
> +	pdata->ioaddr = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(pdata->ioaddr))
> +		return PTR_ERR(pdata->ioaddr);
> +
> +	ioaddr = pdata->ioaddr;
> +
> +	pdata->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(pdata->clk)) {
> +		dev_err(&pdev->dev, "unable to get rtc clock!\n");
> +		return PTR_ERR(pdata->clk);
> +	}
> +	ret = clk_prepare_enable(pdata->clk);
> +	if (ret)
> +		return ret;
>
Is it really necessary to have the clock enabled all the time, or
should it be enabled/disabled for the register accesses only?

> +	/* Configure and enable the RTC */
> +	pdata->irq = platform_get_irq(pdev, 0);
> +	if (pdata->irq >= 0
> +	    && devm_request_irq(&pdev->dev, pdata->irq, mxc_rtc_interrupt,
> +				IRQF_SHARED, pdev->name, pdev) < 0) {
>
If requesting an IRQ with the IRQF_SHARED flag, the interrupt handler
must check whether it was responsible for the IRQ and return IRQ_NONE
if that is not the case to allow some other interrupt handler to jump
in. But AFAICS the RTC IRQ is not shared with any other device, so
requesting the IRQ with IRQF_SHARED is invalid here!

> +		dev_warn(&pdev->dev, "interrupt not available.\n");
> +		pdata->irq = -1;
> +	}
> +
> +	if (pdata->irq >= 0)
> +		device_init_wakeup(&pdev->dev, 1);
> +
> +	/* initialize glitch detect */
> +	__raw_writel(SRTC_LPPDR_INIT, ioaddr + SRTC_LPPDR);
> +	udelay(100);
> +
> +	/* clear lp interrupt status */
> +	__raw_writel(0xFFFFFFFF, ioaddr + SRTC_LPSR);
> +	udelay(100);
> +
> +	/* move out of init state */
> +	__raw_writel((SRTC_LPCR_IE | SRTC_LPCR_NSA), ioaddr + SRTC_LPCR);
> +
> +	udelay(100);
> +
> +	while ((__raw_readl(ioaddr + SRTC_LPSR) & SRTC_LPSR_IES) == 0)
> +		;
>
Loops polling for the change of HW controlled bits should have a
timeout, to prevent locking up when the hardware misbehaves.

> +	/* move out of non-valid state */
> +	__raw_writel((SRTC_LPCR_IE | SRTC_LPCR_NVE | SRTC_LPCR_NSA |
> +		      SRTC_LPCR_EN_LP), ioaddr + SRTC_LPCR);
> +
> +	udelay(100);
> +
> +	while ((__raw_readl(ioaddr + SRTC_LPSR) & SRTC_LPSR_NVES) == 0)
> +		;
>
dto.

> +	__raw_writel(0xFFFFFFFF, ioaddr + SRTC_LPSR);
> +	udelay(100);
> +
> +	platform_set_drvdata(pdev, pdata);
>
The IRQ handler uses platform_get_drvdata(), so this has to be done
BEFORE registering the interrupt handler.

> +	pdata->rtc =
> +	    devm_rtc_device_register(&pdev->dev, pdev->name, &mxc_rtc_ops,
> +				     THIS_MODULE);
> +	if (IS_ERR(pdata->rtc)) {
> +		ret = PTR_ERR(pdata->rtc);
> +		goto exit_put_clk;
> +	}
> +
> +	tv.tv_nsec = 0;
> +	tv.tv_sec = __raw_readl(ioaddr + SRTC_LPSCMR);
> +
> +	/* By default, devices should wakeup if they can */
> +	/* So srtc is set as "should wakeup" as it can */
>
multi line comment style?

> +	device_init_wakeup(&pdev->dev, 1);
> +
> +	return ret;
> +
> +exit_put_clk:
> +	clk_disable_unprepare(pdata->clk);
> +	return ret;
> +}
> +
> +static int __exit mxc_rtc_remove(struct platform_device *pdev)
> +{
> +	struct rtc_drv_data *pdata = platform_get_drvdata(pdev);
> +
> +	clk_disable_unprepare(pdata->clk);
> +	return 0;
> +}
> +
> +/*!
> + * This function is called to save the system time delta relative to
> + * the MXC RTC when enterring a low power state. This time delta is
> + * then used on resume to adjust the system time to account for time
> + * loss while suspended.
> + *
> + * @param   pdev  not used
> + * @param   state Power state to enter.
> + *
> + * @return  The function always returns 0.
> + */
> +static int mxc_rtc_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	struct rtc_drv_data *pdata = platform_get_drvdata(pdev);
> +
> +	if (device_may_wakeup(&pdev->dev))
> +		enable_irq_wake(pdata->irq);
> +
> +	return 0;
> +}
> +
> +/*!
> + * This function is called to correct the system time based on the
> + * current MXC RTC time relative to the time delta saved during
> + * suspend.
> + *
> + * @param   pdev  not used
> + *
> + * @return  The function always returns 0.
> + */
> +static int mxc_rtc_resume(struct platform_device *pdev)
> +{
> +	struct rtc_drv_data *pdata = platform_get_drvdata(pdev);
> +
> +	if (device_may_wakeup(&pdev->dev))
> +		disable_irq_wake(pdata->irq);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id mxc_ids[] = {
> +	{.compatible = "fsl,imx53-rtc",},
>
missing spaces after '{' and before '}'


Lothar Waßmann
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [RFC v2 2/2] backlight: pwm_bl: compute brightness of LED linearly to human eye.
From: Daniel Thompson @ 2017-11-30 11:27 UTC (permalink / raw)
  To: Doug Anderson, Enric Balletbo i Serra
  Cc: Jingoo Han, Richard Purdie, Jacek Anaszewski, Pavel Machek,
	Rob Herring, Brian Norris, Guenter Roeck, Lee Jones,
	Alexandru Stan, linux-leds-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, LKML
In-Reply-To: <CAD=FV=WY-2MmjRxGGaJrQjyeWt+k4_k7j12M4LxAnnZxJF7sXw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>



On 30/11/17 00:44, Doug Anderson wrote:
> Hi,
> 
> On Thu, Nov 16, 2017 at 6:11 AM, Enric Balletbo i Serra
> <enric.balletbo-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org> wrote:
>> When you want to change the brightness using a PWM signal, one thing you
>> need to consider is how human perceive the brightness. Human perceive the
>> brightness change non-linearly, we have better sensitivity at low
>> luminance than high luminance, so to achieve perceived linear dimming, the
>> brightness must be matches to the way our eyes behave. The CIE 1931
>> lightness formula is what actually describes how we perceive light.
>>
>> This patch adds support to compute the brightness levels based on a static
>> table filled with the numbers provided by the CIE 1931 algorithm, for now
>> it only supports PWM resolutions up to 65535 (16 bits) with 1024 steps.
>> Lower PWM resolutions are implemented using the same curve but with less
>> steps, e.g. For a PWM resolution of 256 (8 bits) we have 37 steps.
> 
> Your patch assumes that the input to your formula (luminance, I think)
> scales linearly with PWM duty cycle.  I don't personally know this,
> but has anyone confirmed it's common in reality, or at least is a
> close enough approximation of reality?

Isn't this the loop we went round for v1?

We do know that its not linear, however the graphs from a couple of 
example devices didn't look too scary and nobody has proposed a better 
formula.

At this point the linear interpolation code in patch 1 allows people 
with especially alinear devices to express suitable brightness curves.

However we also know that many DT authors choose not to create good 
brightness tables for their devices... and we'd rather they used allowed 
the kernel to choose a model than to use no model at all.


Daniel.



Enric: BTW sorry I haven't replied so far. That's mostly because
        these looked more "real" and that I should pay them close
        attention (which requires time I haven't had spare to
        consume yet).


>> The calculation of the duty cycle using the CIE 1931 algorithm is enabled by
>> default when you do not define the 'brightness-levels' propriety in your
>> device tree.
> 
> One note is that you probably still want at least a "min" duty cycle.
> I seem to remember some PWM backlights don't work well when the duty
> cycle is too low and it would still be nice to be able to use your
> table.
> 
> 
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
>> ---
>>   drivers/video/backlight/pwm_bl.c | 160 +++++++++++++++++++++++++++++++++++----
>>   include/linux/pwm_backlight.h    |   1 +
>>   2 files changed, 147 insertions(+), 14 deletions(-)
> 
> Something I'd like to see in a patch somewhere in this series is a way
> to expose the backlight "units" to userspace.  As far as I know right
> now the backlight exposed to userspace is "unitless", but it would be
> nice for userspace to query that the backlight is now linear to human
> perception.  For old code, it could always expose the unit as
> "unknown".
> 
> 
>> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
>> index 59b1bfb..ea96358 100644
>> --- a/drivers/video/backlight/pwm_bl.c
>> +++ b/drivers/video/backlight/pwm_bl.c
>> @@ -26,6 +26,112 @@
>>
>>   #define NSTEPS 256
>>
>> +/*
>> + * CIE lightness to PWM conversion table. The CIE 1931 lightness formula is what
>> + * actually describes how we perceive light:
>> + *
>> + *          Y = (L* / 902.3)           if L* ≤ 0.08856
>> + *          Y = ((L* + 16) / 116)^3    if L* > 0.08856
>> + *
>> + * Where Y is the luminance (output) between 0.0 and 1.0, and L* is the
>> + * lightness (input) between 0 and 100.
> 
> Just because I'm stupid and not 100% sure, I think:
> 
> luminance = the amount of light coming out of the screen
> lightness = how bright a human perceives the screen to be
> 
> Is that right?  If so could you add it to the comments?  So "output"
> here is the output to the PWM and "input" is the input from userspace
> (and thus should be expressed in terms of human perception).
> 
> 
>> +       0, 7, 14, 21, 28, 35, 43, 50, 57, 64, 71, 78, 85, 92, 99, 106, 114, 121,
> 
> Seems like you could save space (and nicely use the previous patch) by
> using the linear interpolation code from the previous patch, since
> 
> 0 + 7 = 7
> + 7 = 14
> + 7 = 21
> + 7 = 28
> + 7 = 35
> 
> ...and it would likely be OK to keep going and be slight off, so:
> 
> + 7 = 42
> + 7 = 49
> + 7 = 56
> + 7 = 63
> + 7 = 70
> ...
> ...
> 
> In other words it seems like you're just providing a default table...
> 
> -Doug
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v2 1/8] ASoC: tlv320aic31xx: Fix typo in DT binding documentation
From: Mark Brown @ 2017-11-30 11:43 UTC (permalink / raw)
  To: Andrew F. Davis
  Cc: Liam Girdwood, Rob Herring, Mark Rutland, Benoît Cousson,
	Tony Lindgren, Shawn Guo, Sascha Hauer,
	alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20171129171359.29382-2-afd-l0cyMroinI0@public.gmane.org>

[-- Attachment #1: Type: text/plain, Size: 589 bytes --]

On Wed, Nov 29, 2017 at 11:13:52AM -0600, Andrew F. Davis wrote:
> The property used to specify a GPIO intended for reset is "reset-gpios",
> this binding uses "gpio-reset", as almost all other bindings use the
> former name this use of the latter was certainly not intended. It is not
> compatible with newer methods used to fetch GPIO pins and to prevent the
> spread of this error to other bindings lets fix this here.

When you say you're fixing a typo I'm looking for a fix for something
like two letters being transposed.  This isn't that, it's a completely
different property name.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply

* [PATCH v3 0/3] drm/omap: Support for dispc memory bandwidth limit
From: Peter Ujfalusi @ 2017-11-30 12:12 UTC (permalink / raw)
  To: tomi.valkeinen, laurent.pinchart; +Cc: airlied, devicetree, jsarha, dri-devel

Hi,

Changes since v2:
- Rebased on drm-next (v2 was based on drm-next and my
  'drm/omap: Module parameter for display order configuration' series, thus it
  was not applying cleanly.
- Added Acked-by from Rob to the dt-binding changes

Changes since v1:
- Use the crtc->mode_valid and not connector->mode_valid as the limit is really
  posed by the 'crtc' (DISPC)
- Bandwidth calculation changed: do the calculation in place + extended comment
- looked for better place to document the max-memory-bandwidth, but
  can not find any suitable document. Creating a new txt file for only one
  property did not seemed feasable.

This series will add simple memory bandwidth limit support to reject modes
which, if used with one plane in full size would fail the limit.

Regards,
Peter
---
Peter Ujfalusi (3):
  dt-bindings: display/ti: Add optional property to set memory bandwidth
    limit
  drm/omap: dss: Add support for reporting memory bandwidth limitation
  drm/omap: Filter displays mode based on bandwidth limit

 .../devicetree/bindings/display/ti/ti,dra7-dss.txt |  5 +++
 .../bindings/display/ti/ti,omap2-dss.txt           |  4 +++
 .../bindings/display/ti/ti,omap3-dss.txt           |  4 +++
 .../bindings/display/ti/ti,omap4-dss.txt           |  4 +++
 .../bindings/display/ti/ti,omap5-dss.txt           |  4 +++
 drivers/gpu/drm/omapdrm/dss/dispc.c                | 13 ++++++++
 drivers/gpu/drm/omapdrm/dss/omapdss.h              |  2 ++
 drivers/gpu/drm/omapdrm/omap_crtc.c                | 37 ++++++++++++++++++++++
 drivers/gpu/drm/omapdrm/omap_drv.c                 |  5 +++
 drivers/gpu/drm/omapdrm/omap_drv.h                 |  3 ++
 10 files changed, 81 insertions(+)

-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply

* [PATCH v3 1/3] dt-bindings: display/ti: Add optional property to set memory bandwidth limit
From: Peter Ujfalusi @ 2017-11-30 12:12 UTC (permalink / raw)
  To: tomi.valkeinen, laurent.pinchart; +Cc: airlied, devicetree, jsarha, dri-devel
In-Reply-To: <20171130121237.30431-1-peter.ujfalusi@ti.com>

max-memory-bandwidth can be used to specify the maximum bandwidth dispc
can use when reading display data from main memory.

In some SoC (am437x for example) we have memory bandwidth limitation
which causes underflow in the display subsystem.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/display/ti/ti,dra7-dss.txt  | 5 +++++
 Documentation/devicetree/bindings/display/ti/ti,omap2-dss.txt | 4 ++++
 Documentation/devicetree/bindings/display/ti/ti,omap3-dss.txt | 4 ++++
 Documentation/devicetree/bindings/display/ti/ti,omap4-dss.txt | 4 ++++
 Documentation/devicetree/bindings/display/ti/ti,omap5-dss.txt | 4 ++++
 5 files changed, 21 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/ti/ti,dra7-dss.txt b/Documentation/devicetree/bindings/display/ti/ti,dra7-dss.txt
index c30f9ec189ed..91279f1060fe 100644
--- a/Documentation/devicetree/bindings/display/ti/ti,dra7-dss.txt
+++ b/Documentation/devicetree/bindings/display/ti/ti,dra7-dss.txt
@@ -47,6 +47,11 @@ Required properties:
 - clocks: handle to fclk
 - clock-names: "fck"
 
+Optional properties:
+- max-memory-bandwidth: Input memory (from main memory to dispc) bandwidth limit
+			in bytes per second
+
+
 HDMI
 ----
 
diff --git a/Documentation/devicetree/bindings/display/ti/ti,omap2-dss.txt b/Documentation/devicetree/bindings/display/ti/ti,omap2-dss.txt
index afcd5a86c6a4..ee867c4d1152 100644
--- a/Documentation/devicetree/bindings/display/ti/ti,omap2-dss.txt
+++ b/Documentation/devicetree/bindings/display/ti/ti,omap2-dss.txt
@@ -28,6 +28,10 @@ Required properties:
 - ti,hwmods: "dss_dispc"
 - interrupts: the DISPC interrupt
 
+Optional properties:
+- max-memory-bandwidth: Input memory (from main memory to dispc) bandwidth limit
+			in bytes per second
+
 
 RFBI
 ----
diff --git a/Documentation/devicetree/bindings/display/ti/ti,omap3-dss.txt b/Documentation/devicetree/bindings/display/ti/ti,omap3-dss.txt
index dc66e1447c31..cd02516a40b6 100644
--- a/Documentation/devicetree/bindings/display/ti/ti,omap3-dss.txt
+++ b/Documentation/devicetree/bindings/display/ti/ti,omap3-dss.txt
@@ -37,6 +37,10 @@ Required properties:
 - clocks: handle to fclk
 - clock-names: "fck"
 
+Optional properties:
+- max-memory-bandwidth: Input memory (from main memory to dispc) bandwidth limit
+			in bytes per second
+
 
 RFBI
 ----
diff --git a/Documentation/devicetree/bindings/display/ti/ti,omap4-dss.txt b/Documentation/devicetree/bindings/display/ti/ti,omap4-dss.txt
index bc624db8888d..0f85f6b3a5a8 100644
--- a/Documentation/devicetree/bindings/display/ti/ti,omap4-dss.txt
+++ b/Documentation/devicetree/bindings/display/ti/ti,omap4-dss.txt
@@ -36,6 +36,10 @@ Required properties:
 - clocks: handle to fclk
 - clock-names: "fck"
 
+Optional properties:
+- max-memory-bandwidth: Input memory (from main memory to dispc) bandwidth limit
+			in bytes per second
+
 
 RFBI
 ----
diff --git a/Documentation/devicetree/bindings/display/ti/ti,omap5-dss.txt b/Documentation/devicetree/bindings/display/ti/ti,omap5-dss.txt
index 118a486c47bb..20861218649f 100644
--- a/Documentation/devicetree/bindings/display/ti/ti,omap5-dss.txt
+++ b/Documentation/devicetree/bindings/display/ti/ti,omap5-dss.txt
@@ -36,6 +36,10 @@ Required properties:
 - clocks: handle to fclk
 - clock-names: "fck"
 
+Optional properties:
+- max-memory-bandwidth: Input memory (from main memory to dispc) bandwidth limit
+			in bytes per second
+
 
 RFBI
 ----
-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox