diff mbox series

[v3,6/7] soc: renesas: rmobile-sysc: Convert to readl_poll_timeout_atomic()

Message ID ae4bf03ab8fd5a557c683086958d6764babc0723.1685692810.git.geert+renesas@glider.be (mailing list archive)
State New, archived
Headers show
Series iopoll: Busy loop and timeout improvements + conversions | expand

Commit Message

Geert Uytterhoeven June 2, 2023, 8:50 a.m. UTC 
Use readl_poll_timeout_atomic() instead of open-coding the same
operation.

  1. rmobile_pd_power_down(): as typically less than 20 retries are
     needed, PSTR_RETRIES (100) µs is a suitable timeout value.

  2. __rmobile_pd_power_up(): the old method of first polling some
     cycles with a 1 µs delay, followed by more polling cycles without
     any delay didn't make much sense, as the latter was insignificant
     compared to the former.  Furthermore, typically no retries are
     needed.  Hence just retain the polling with delay.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Polling measurements done on R-Mobile APE6 and A1, and SH-Mobile AG5.

v3:
  - New.
---
 drivers/soc/renesas/rmobile-sysc.c | 31 ++++++++++--------------------
 1 file changed, 10 insertions(+), 21 deletions(-)

Comments

kernel test robot June 2, 2023, 1:51 p.m. UTC | #1 
Hi Geert,

kernel test robot noticed the following build warnings:

[auto build test WARNING on geert-renesas-drivers/renesas-clk]
[also build test WARNING on joro-iommu/next clk/clk-next linus/master v6.4-rc4 next-20230602]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Geert-Uytterhoeven/iopoll-Call-cpu_relax-in-busy-loops/20230602-165454
base:   https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git renesas-clk
patch link:    https://lore.kernel.org/r/ae4bf03ab8fd5a557c683086958d6764babc0723.1685692810.git.geert%2Brenesas%40glider.be
patch subject: [PATCH v3 6/7] soc: renesas: rmobile-sysc: Convert to readl_poll_timeout_atomic()
config: hexagon-randconfig-r031-20230531 (https://download.01.org/0day-ci/archive/20230602/202306022137.eAP4Q3hL-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 4faf3aaf28226a4e950c103a14f6fc1d1fdabb1b)
reproduce (this is a W=1 build):
        mkdir -p ~/bin
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/2198e2f3fd21eb62861b6ada08eeec7d6f420ee3
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Geert-Uytterhoeven/iopoll-Call-cpu_relax-in-busy-loops/20230602-165454
        git checkout 2198e2f3fd21eb62861b6ada08eeec7d6f420ee3
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=hexagon olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/soc/renesas/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306022137.eAP4Q3hL-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/soc/renesas/rmobile-sysc.c:15:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
                                                     ^
   In file included from drivers/soc/renesas/rmobile-sysc.c:15:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
                                                     ^
   In file included from drivers/soc/renesas/rmobile-sysc.c:15:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
>> drivers/soc/renesas/rmobile-sysc.c:78:10: warning: variable 'ret' is uninitialized when used here [-Wuninitialized]
                   return ret;
                          ^~~
   drivers/soc/renesas/rmobile-sysc.c:75:9: note: initialize the variable 'ret' to silence this warning
           int ret;
                  ^
                   = 0
   7 warnings generated.


vim +/ret +78 drivers/soc/renesas/rmobile-sysc.c

8f45b112fc66ef arch/arm/mach-shmobile/pm-rmobile.c Kuninori Morimoto  2012-07-05  71  
445aeb081bc713 arch/arm/mach-shmobile/pm-rmobile.c Geert Uytterhoeven 2018-11-29  72  static int __rmobile_pd_power_up(struct rmobile_pm_domain *rmobile_pd)
8f45b112fc66ef arch/arm/mach-shmobile/pm-rmobile.c Kuninori Morimoto  2012-07-05  73  {
2198e2f3fd21eb drivers/soc/renesas/rmobile-sysc.c  Geert Uytterhoeven 2023-06-02  74  	unsigned int val, mask = BIT(rmobile_pd->bit_shift);
2198e2f3fd21eb drivers/soc/renesas/rmobile-sysc.c  Geert Uytterhoeven 2023-06-02  75  	int ret;
8f45b112fc66ef arch/arm/mach-shmobile/pm-rmobile.c Kuninori Morimoto  2012-07-05  76  
8b6bed678428b6 drivers/soc/renesas/rmobile-sysc.c  Geert Uytterhoeven 2020-11-19  77  	if (readl(rmobile_pd->base + PSTR) & mask)
445aeb081bc713 arch/arm/mach-shmobile/pm-rmobile.c Geert Uytterhoeven 2018-11-29 @78  		return ret;
8f45b112fc66ef arch/arm/mach-shmobile/pm-rmobile.c Kuninori Morimoto  2012-07-05  79  
8b6bed678428b6 drivers/soc/renesas/rmobile-sysc.c  Geert Uytterhoeven 2020-11-19  80  	writel(mask, rmobile_pd->base + SWUCR);
8f45b112fc66ef arch/arm/mach-shmobile/pm-rmobile.c Kuninori Morimoto  2012-07-05  81  
2198e2f3fd21eb drivers/soc/renesas/rmobile-sysc.c  Geert Uytterhoeven 2023-06-02  82  	ret = readl_poll_timeout_atomic(rmobile_pd->base + SWUCR, val,
2198e2f3fd21eb drivers/soc/renesas/rmobile-sysc.c  Geert Uytterhoeven 2023-06-02  83  					(val & mask), PSTR_DELAY_US,
2198e2f3fd21eb drivers/soc/renesas/rmobile-sysc.c  Geert Uytterhoeven 2023-06-02  84  					PSTR_RETRIES * PSTR_DELAY_US);
8f45b112fc66ef arch/arm/mach-shmobile/pm-rmobile.c Kuninori Morimoto  2012-07-05  85  
8f45b112fc66ef arch/arm/mach-shmobile/pm-rmobile.c Kuninori Morimoto  2012-07-05  86  	pr_debug("%s: Power on, 0x%08x -> PSTR = 0x%08x\n",
25717b85736076 arch/arm/mach-shmobile/pm-rmobile.c Geert Uytterhoeven 2014-12-03  87  		 rmobile_pd->genpd.name, mask,
8b6bed678428b6 drivers/soc/renesas/rmobile-sysc.c  Geert Uytterhoeven 2020-11-19  88  		 readl(rmobile_pd->base + PSTR));
8f45b112fc66ef arch/arm/mach-shmobile/pm-rmobile.c Kuninori Morimoto  2012-07-05  89  
8f45b112fc66ef arch/arm/mach-shmobile/pm-rmobile.c Kuninori Morimoto  2012-07-05  90  	return ret;
8f45b112fc66ef arch/arm/mach-shmobile/pm-rmobile.c Kuninori Morimoto  2012-07-05  91  }
8f45b112fc66ef arch/arm/mach-shmobile/pm-rmobile.c Kuninori Morimoto  2012-07-05  92
Geert Uytterhoeven June 5, 2023, 1:15 p.m. UTC | #2 
On Fri, Jun 2, 2023 at 10:51 AM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
> Use readl_poll_timeout_atomic() instead of open-coding the same
> operation.
>
>   1. rmobile_pd_power_down(): as typically less than 20 retries are
>      needed, PSTR_RETRIES (100) µs is a suitable timeout value.
>
>   2. __rmobile_pd_power_up(): the old method of first polling some
>      cycles with a 1 µs delay, followed by more polling cycles without
>      any delay didn't make much sense, as the latter was insignificant
>      compared to the former.  Furthermore, typically no retries are
>      needed.  Hence just retain the polling with delay.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

> diff --git a/drivers/soc/renesas/rmobile-sysc.c b/drivers/soc/renesas/rmobile-sysc.c
> index 728ebac98e14a5cc..5d621c35fba1116a 100644

> @@ -74,25 +71,17 @@ static int rmobile_pd_power_down(struct generic_pm_domain *genpd)
>
>  static int __rmobile_pd_power_up(struct rmobile_pm_domain *rmobile_pd)
>  {
> -       unsigned int mask = BIT(rmobile_pd->bit_shift);
> -       unsigned int retry_count;
> -       int ret = 0;
> +       unsigned int val, mask = BIT(rmobile_pd->bit_shift);
> +       int ret;

Oops, "ret" should still be initialized to zero.

>
>         if (readl(rmobile_pd->base + PSTR) & mask)
>                 return ret;
>
>         writel(mask, rmobile_pd->base + SWUCR);
>
> -       for (retry_count = 2 * PSTR_RETRIES; retry_count; retry_count--) {
> -               if (!(readl(rmobile_pd->base + SWUCR) & mask))
> -                       break;
> -               if (retry_count > PSTR_RETRIES)
> -                       udelay(PSTR_DELAY_US);
> -               else
> -                       cpu_relax();
> -       }
> -       if (!retry_count)
> -               ret = -EIO;
> +       ret = readl_poll_timeout_atomic(rmobile_pd->base + SWUCR, val,
> +                                       (val & mask), PSTR_DELAY_US,
> +                                       PSTR_RETRIES * PSTR_DELAY_US);
>
>         pr_debug("%s: Power on, 0x%08x -> PSTR = 0x%08x\n",
>                  rmobile_pd->genpd.name, mask,

Gr{oetje,eeting}s,

                        Geert
diff mbox series

Patch

diff --git a/drivers/soc/renesas/rmobile-sysc.c b/drivers/soc/renesas/rmobile-sysc.c
index 728ebac98e14a5cc..5d621c35fba1116a 100644
--- a/drivers/soc/renesas/rmobile-sysc.c
+++ b/drivers/soc/renesas/rmobile-sysc.c
@@ -12,6 +12,8 @@ 
 #include <linux/clk/renesas.h>
 #include <linux/console.h>
 #include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/pm.h>
@@ -19,8 +21,6 @@ 
 #include <linux/pm_domain.h>
 #include <linux/slab.h>
 
-#include <asm/io.h>
-
 /* SYSC */
 #define SPDCR		0x08	/* SYS Power Down Control Register */
 #define SWUCR		0x14	/* SYS Wakeup Control Register */
@@ -47,6 +47,7 @@  static int rmobile_pd_power_down(struct generic_pm_domain *genpd)
 {
 	struct rmobile_pm_domain *rmobile_pd = to_rmobile_pd(genpd);
 	unsigned int mask = BIT(rmobile_pd->bit_shift);
+	u32 val;
 
 	if (rmobile_pd->suspend) {
 		int ret = rmobile_pd->suspend();
@@ -56,14 +57,10 @@  static int rmobile_pd_power_down(struct generic_pm_domain *genpd)
 	}
 
 	if (readl(rmobile_pd->base + PSTR) & mask) {
-		unsigned int retry_count;
 		writel(mask, rmobile_pd->base + SPDCR);
 
-		for (retry_count = PSTR_RETRIES; retry_count; retry_count--) {
-			if (!(readl(rmobile_pd->base + SPDCR) & mask))
-				break;
-			cpu_relax();
-		}
+		readl_poll_timeout_atomic(rmobile_pd->base + SPDCR, val,
+					  !(val & mask), 0, PSTR_RETRIES);
 	}
 
 	pr_debug("%s: Power off, 0x%08x -> PSTR = 0x%08x\n", genpd->name, mask,
@@ -74,25 +71,17 @@  static int rmobile_pd_power_down(struct generic_pm_domain *genpd)
 
 static int __rmobile_pd_power_up(struct rmobile_pm_domain *rmobile_pd)
 {
-	unsigned int mask = BIT(rmobile_pd->bit_shift);
-	unsigned int retry_count;
-	int ret = 0;
+	unsigned int val, mask = BIT(rmobile_pd->bit_shift);
+	int ret;
 
 	if (readl(rmobile_pd->base + PSTR) & mask)
 		return ret;
 
 	writel(mask, rmobile_pd->base + SWUCR);
 
-	for (retry_count = 2 * PSTR_RETRIES; retry_count; retry_count--) {
-		if (!(readl(rmobile_pd->base + SWUCR) & mask))
-			break;
-		if (retry_count > PSTR_RETRIES)
-			udelay(PSTR_DELAY_US);
-		else
-			cpu_relax();
-	}
-	if (!retry_count)
-		ret = -EIO;
+	ret = readl_poll_timeout_atomic(rmobile_pd->base + SWUCR, val,
+					(val & mask), PSTR_DELAY_US,
+					PSTR_RETRIES * PSTR_DELAY_US);
 
 	pr_debug("%s: Power on, 0x%08x -> PSTR = 0x%08x\n",
 		 rmobile_pd->genpd.name, mask,