| Message ID | 1596034117-24246-2-git-send-email-Anson.Huang@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support building i.MX ARMv8 platforms clock driver as module | expand |
On 7/29/20 7:48 AM, Anson Huang wrote: > Use readl_relaxed() instead of __raw_readl(), and use BIT(x) > instead of (1 << X) to fix below build warning reported by kernel > test robot: > > drivers/clk/imx/clk-imx6sl.c:149:49: warning: Shifting signed 32-bit > value by 31 bits is undefined behaviour [shiftTooManyBitsSigned] > while (!(__raw_readl(anatop_base + PLL_ARM) & BM_PLL_ARM_LOCK)) > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > Reported-by: kernel test robot <lkp@intel.com> > --- > Changes since V6: > - improve the subject. > --- > drivers/clk/imx/clk-imx6sl.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/drivers/clk/imx/clk-imx6sl.c b/drivers/clk/imx/clk-imx6sl.c > index 0f647d1..e69dba1 100644 > --- a/drivers/clk/imx/clk-imx6sl.c > +++ b/drivers/clk/imx/clk-imx6sl.c > @@ -3,6 +3,7 @@ > * Copyright 2013-2014 Freescale Semiconductor, Inc. > */ > > +#include <linux/bitfield.h> Hi, I think you want #include <linux/bits.h> for BIT() usage. > #include <linux/clk.h> > #include <linux/clkdev.h> > #include <linux/err.h> > @@ -14,19 +15,19 @@ > #include "clk.h" > > #define CCSR 0xc > -#define BM_CCSR_PLL1_SW_CLK_SEL (1 << 2) > +#define BM_CCSR_PLL1_SW_CLK_SEL BIT(2) > #define CACRR 0x10 > #define CDHIPR 0x48 > -#define BM_CDHIPR_ARM_PODF_BUSY (1 << 16) > +#define BM_CDHIPR_ARM_PODF_BUSY BIT(16) > #define ARM_WAIT_DIV_396M 2 > #define ARM_WAIT_DIV_792M 4 > #define ARM_WAIT_DIV_996M 6 > > #define PLL_ARM 0x0 > -#define BM_PLL_ARM_DIV_SELECT (0x7f << 0) > -#define BM_PLL_ARM_POWERDOWN (1 << 12) > -#define BM_PLL_ARM_ENABLE (1 << 13) > -#define BM_PLL_ARM_LOCK (1 << 31) > +#define BM_PLL_ARM_DIV_SELECT 0x7f > +#define BM_PLL_ARM_POWERDOWN BIT(12) > +#define BM_PLL_ARM_ENABLE BIT(13) > +#define BM_PLL_ARM_LOCK BIT(31) > #define PLL_ARM_DIV_792M 66 > > static const char *step_sels[] = { "osc", "pll2_pfd2", }; thanks.
Hi, Randy > Subject: Re: [PATCH V7 1/6] clk: imx6sl: Use BIT(x) to avoid shifting signed > 32-bit value by 31 bits > > On 7/29/20 7:48 AM, Anson Huang wrote: > > Use readl_relaxed() instead of __raw_readl(), and use BIT(x) instead > > of (1 << X) to fix below build warning reported by kernel test robot: > > > > drivers/clk/imx/clk-imx6sl.c:149:49: warning: Shifting signed 32-bit > > value by 31 bits is undefined behaviour [shiftTooManyBitsSigned] > > while (!(__raw_readl(anatop_base + PLL_ARM) & > BM_PLL_ARM_LOCK)) > > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > > Reported-by: kernel test robot <lkp@intel.com> > > --- > > Changes since V6: > > - improve the subject. > > --- > > drivers/clk/imx/clk-imx6sl.c | 15 ++++++++------- > > 1 file changed, 8 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/clk/imx/clk-imx6sl.c > > b/drivers/clk/imx/clk-imx6sl.c index 0f647d1..e69dba1 100644 > > --- a/drivers/clk/imx/clk-imx6sl.c > > +++ b/drivers/clk/imx/clk-imx6sl.c > > @@ -3,6 +3,7 @@ > > * Copyright 2013-2014 Freescale Semiconductor, Inc. > > */ > > > > +#include <linux/bitfield.h> > > Hi, > I think you want > #include <linux/bits.h> > > for BIT() usage. Actually, the linux/of.h already includes linux/bitops.h and linux/bitops.h includes linux/bits.h, so I will just drop linux/bitfield.h and send a V8. Thanks, Anson
On 7/29/20 4:51 PM, Anson Huang wrote: > Hi, Randy > > >> Subject: Re: [PATCH V7 1/6] clk: imx6sl: Use BIT(x) to avoid shifting signed >> 32-bit value by 31 bits >> >> On 7/29/20 7:48 AM, Anson Huang wrote: >>> Use readl_relaxed() instead of __raw_readl(), and use BIT(x) instead >>> of (1 << X) to fix below build warning reported by kernel test robot: >>> >>> drivers/clk/imx/clk-imx6sl.c:149:49: warning: Shifting signed 32-bit >>> value by 31 bits is undefined behaviour [shiftTooManyBitsSigned] >>> while (!(__raw_readl(anatop_base + PLL_ARM) & >> BM_PLL_ARM_LOCK)) >>> >>> Signed-off-by: Anson Huang <Anson.Huang@nxp.com> >>> Reported-by: kernel test robot <lkp@intel.com> >>> --- >>> Changes since V6: >>> - improve the subject. >>> --- >>> drivers/clk/imx/clk-imx6sl.c | 15 ++++++++------- >>> 1 file changed, 8 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/clk/imx/clk-imx6sl.c >>> b/drivers/clk/imx/clk-imx6sl.c index 0f647d1..e69dba1 100644 >>> --- a/drivers/clk/imx/clk-imx6sl.c >>> +++ b/drivers/clk/imx/clk-imx6sl.c >>> @@ -3,6 +3,7 @@ >>> * Copyright 2013-2014 Freescale Semiconductor, Inc. >>> */ >>> >>> +#include <linux/bitfield.h> >> >> Hi, >> I think you want >> #include <linux/bits.h> >> >> for BIT() usage. > > Actually, the linux/of.h already includes linux/bitops.h and linux/bitops.h includes > linux/bits.h, so I will just drop linux/bitfield.h and send a V8. or you could read Documentation/process/submit-checklist.rst, where rule #1 says: 1) If you use a facility then #include the file that defines/declares that facility. Don't depend on other header files pulling in ones that you use.
Hi, Randy > Subject: Re: [PATCH V7 1/6] clk: imx6sl: Use BIT(x) to avoid shifting signed > 32-bit value by 31 bits > > On 7/29/20 4:51 PM, Anson Huang wrote: > > Hi, Randy > > > > > >> Subject: Re: [PATCH V7 1/6] clk: imx6sl: Use BIT(x) to avoid shifting > >> signed 32-bit value by 31 bits > >> > >> On 7/29/20 7:48 AM, Anson Huang wrote: > >>> Use readl_relaxed() instead of __raw_readl(), and use BIT(x) instead > >>> of (1 << X) to fix below build warning reported by kernel test robot: > >>> > >>> drivers/clk/imx/clk-imx6sl.c:149:49: warning: Shifting signed 32-bit > >>> value by 31 bits is undefined behaviour [shiftTooManyBitsSigned] > >>> while (!(__raw_readl(anatop_base + PLL_ARM) & > >> BM_PLL_ARM_LOCK)) > >>> > >>> Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > >>> Reported-by: kernel test robot <lkp@intel.com> > >>> --- > >>> Changes since V6: > >>> - improve the subject. > >>> --- > >>> drivers/clk/imx/clk-imx6sl.c | 15 ++++++++------- > >>> 1 file changed, 8 insertions(+), 7 deletions(-) > >>> > >>> diff --git a/drivers/clk/imx/clk-imx6sl.c > >>> b/drivers/clk/imx/clk-imx6sl.c index 0f647d1..e69dba1 100644 > >>> --- a/drivers/clk/imx/clk-imx6sl.c > >>> +++ b/drivers/clk/imx/clk-imx6sl.c > >>> @@ -3,6 +3,7 @@ > >>> * Copyright 2013-2014 Freescale Semiconductor, Inc. > >>> */ > >>> > >>> +#include <linux/bitfield.h> > >> > >> Hi, > >> I think you want > >> #include <linux/bits.h> > >> > >> for BIT() usage. > > > > Actually, the linux/of.h already includes linux/bitops.h and > > linux/bitops.h includes linux/bits.h, so I will just drop linux/bitfield.h and > send a V8. > > or you could read Documentation/process/submit-checklist.rst, > where rule #1 says: > > 1) If you use a facility then #include the file that defines/declares > that facility. Don't depend on other header files pulling in ones > that you use. Understood, while I search "BIT()" in clk driver, most of the drivers does NOT include linux/bits.h even they use it. But OK, I will send V9 to include it. Thanks, Anson
On Thu, Jul 30, 2020 at 3:14 AM Anson Huang <anson.huang@nxp.com> wrote: > > Subject: Re: [PATCH V7 1/6] clk: imx6sl: Use BIT(x) to avoid shifting signed > > 32-bit value by 31 bits > > or you could read Documentation/process/submit-checklist.rst, > > where rule #1 says: > > > > 1) If you use a facility then #include the file that defines/declares > > that facility. Don't depend on other header files pulling in ones > > that you use. > > Understood, while I search "BIT()" in clk driver, most of the drivers does NOT include > linux/bits.h even they use it. > > But OK, I will send V9 to include it. Ok, good. I have a patch series that adds it to a lot of files. Note that linux/bitops.h itself is a fairly big header with everything else that it includes, and it is included almost everywhere indirectly. When we change the other headers to not require linux/bitops.h any more, everything that uses BIT() needs to include linux/bits.h. Arnd
diff --git a/drivers/clk/imx/clk-imx6sl.c b/drivers/clk/imx/clk-imx6sl.c index 0f647d1..e69dba1 100644 --- a/drivers/clk/imx/clk-imx6sl.c +++ b/drivers/clk/imx/clk-imx6sl.c @@ -3,6 +3,7 @@ * Copyright 2013-2014 Freescale Semiconductor, Inc. */ +#include <linux/bitfield.h> #include <linux/clk.h> #include <linux/clkdev.h> #include <linux/err.h> @@ -14,19 +15,19 @@ #include "clk.h" #define CCSR 0xc -#define BM_CCSR_PLL1_SW_CLK_SEL (1 << 2) +#define BM_CCSR_PLL1_SW_CLK_SEL BIT(2) #define CACRR 0x10 #define CDHIPR 0x48 -#define BM_CDHIPR_ARM_PODF_BUSY (1 << 16) +#define BM_CDHIPR_ARM_PODF_BUSY BIT(16) #define ARM_WAIT_DIV_396M 2 #define ARM_WAIT_DIV_792M 4 #define ARM_WAIT_DIV_996M 6 #define PLL_ARM 0x0 -#define BM_PLL_ARM_DIV_SELECT (0x7f << 0) -#define BM_PLL_ARM_POWERDOWN (1 << 12) -#define BM_PLL_ARM_ENABLE (1 << 13) -#define BM_PLL_ARM_LOCK (1 << 31) +#define BM_PLL_ARM_DIV_SELECT 0x7f +#define BM_PLL_ARM_POWERDOWN BIT(12) +#define BM_PLL_ARM_ENABLE BIT(13) +#define BM_PLL_ARM_LOCK BIT(31) #define PLL_ARM_DIV_792M 66 static const char *step_sels[] = { "osc", "pll2_pfd2", }; @@ -145,7 +146,7 @@ static void imx6sl_enable_pll_arm(bool enable) val |= BM_PLL_ARM_ENABLE; val &= ~BM_PLL_ARM_POWERDOWN; writel_relaxed(val, anatop_base + PLL_ARM); - while (!(__raw_readl(anatop_base + PLL_ARM) & BM_PLL_ARM_LOCK)) + while (!(readl_relaxed(anatop_base + PLL_ARM) & BM_PLL_ARM_LOCK)) ; } else { writel_relaxed(saved_pll_arm, anatop_base + PLL_ARM);
Use readl_relaxed() instead of __raw_readl(), and use BIT(x) instead of (1 << X) to fix below build warning reported by kernel test robot: drivers/clk/imx/clk-imx6sl.c:149:49: warning: Shifting signed 32-bit value by 31 bits is undefined behaviour [shiftTooManyBitsSigned] while (!(__raw_readl(anatop_base + PLL_ARM) & BM_PLL_ARM_LOCK)) Signed-off-by: Anson Huang <Anson.Huang@nxp.com> Reported-by: kernel test robot <lkp@intel.com> --- Changes since V6: - improve the subject. --- drivers/clk/imx/clk-imx6sl.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)