| Message ID | 1441626234-16364-1-git-send-email-andreas.devel@googlemail.com |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
On 07.09.2015 13:43, Andreas Bießmann wrote: > From: Heiko Schocher <hs@denx.de> > > introduce BIT() definition, used in at91_udc gadget > driver. > > Signed-off-by: Heiko Schocher <hs@denx.de> > [remove all other occurrences of BIT(x) definition] > Signed-off-by: Andreas Bießmann <andreas.devel@googlemail.com> > --- > Full buildman is running > > Would be nice to get some Acked-by/Reviewed-by since this is a fixup of one > patch in a series that should go into 2015.10. Thanks Andreas for cleaning this up. I didn't compile test this yet, but for the mvebu related stuff: Acked-by: Stefan Roese <sr@denx.de> Thanks, Stefan
Hi, On Mon, 7 Sep 2015 13:43:52 +0200 Andreas Bießmann <andreas.devel@googlemail.com> wrote: > From: Heiko Schocher <hs@denx.de> > > introduce BIT() definition, used in at91_udc gadget > driver. > > Signed-off-by: Heiko Schocher <hs@denx.de> > [remove all other occurrences of BIT(x) definition] > Signed-off-by: Andreas Bießmann <andreas.devel@googlemail.com> for anx9804 video driver Acked-by: Anatolij Gustschin <agust@denx.de> Thanks, Anatolij
Hi, Andreas On 07.09.15 14:43, Andreas Bießmann wrote: > From: Heiko Schocher <hs@denx.de> > > introduce BIT() definition, used in at91_udc gadget > driver. > > Signed-off-by: Heiko Schocher <hs@denx.de> > [remove all other occurrences of BIT(x) definition] > Signed-off-by: Andreas Bießmann <andreas.devel@googlemail.com> > --- > Full buildman is running > .... > > +#define BIT(nr) (1UL << (nr)) Why UL? Why not simply 1 << (nr)? What if I need set ULL bit on 32-bit system? Thanks for explanation. > + > /* > * ffs: find first bit set. This is defined the same way as > * the libc and compiler builtin ffs routines, therefore >
On Tue, 2015-09-08 at 21:01 +0300, ivan.khoronzhuk wrote: > Hi, Andreas > > On 07.09.15 14:43, Andreas Bießmann wrote: > > From: Heiko Schocher <hs@denx.de> > > > > introduce BIT() definition, used in at91_udc gadget > > driver. > > > > Signed-off-by: Heiko Schocher <hs@denx.de> > > [remove all other occurrences of BIT(x) definition] > > Signed-off-by: Andreas Bießmann <andreas.devel@googlemail.com> > > --- > > Full buildman is running > > > > .... > > > > > +#define BIT(nr) (1UL << (nr)) > > Why UL? Why not simply 1 << (nr)? That would give the wrong result for nr == 31 if used as a 64-bit number, and would produce undefined behavior for nr >= 32 (though even with 1UL that would be undefined on 32-bit builds). > What if I need set ULL bit on 32-bit system? > Thanks for explanation. Yes, ULL would be better. -Scott
On Wed, Sep 09, 2015 at 11:22:25AM -0500, Scott Wood wrote: > On Tue, 2015-09-08 at 21:01 +0300, ivan.khoronzhuk wrote: > > Hi, Andreas > > > > On 07.09.15 14:43, Andreas Bießmann wrote: > > > From: Heiko Schocher <hs@denx.de> > > > > > > introduce BIT() definition, used in at91_udc gadget > > > driver. > > > > > > Signed-off-by: Heiko Schocher <hs@denx.de> > > > [remove all other occurrences of BIT(x) definition] > > > Signed-off-by: Andreas Bießmann <andreas.devel@googlemail.com> > > > --- > > > Full buildman is running > > > > > > > .... > > > > > > > > +#define BIT(nr) (1UL << (nr)) > > > > Why UL? Why not simply 1 << (nr)? > > That would give the wrong result for nr == 31 if used as a 64-bit number, and > would produce undefined behavior for nr >= 32 (though even with 1UL that > would be undefined on 32-bit builds). > > > What if I need set ULL bit on 32-bit system? > > Thanks for explanation. > > Yes, ULL would be better. That would be BIT_ULL(nr) ? I want to assume that there was some care given upstream here. It was about 2 years ago now the kernel added a specific BIT_ULL and family in addition to BIT(nr) from back in 2007.
On Wed, 2015-09-09 at 12:37 -0400, Tom Rini wrote: > On Wed, Sep 09, 2015 at 11:22:25AM -0500, Scott Wood wrote: > > On Tue, 2015-09-08 at 21:01 +0300, ivan.khoronzhuk wrote: > > > Hi, Andreas > > > > > > On 07.09.15 14:43, Andreas Bießmann wrote: > > > > From: Heiko Schocher <hs@denx.de> > > > > > > > > introduce BIT() definition, used in at91_udc gadget > > > > driver. > > > > > > > > Signed-off-by: Heiko Schocher <hs@denx.de> > > > > [remove all other occurrences of BIT(x) definition] > > > > Signed-off-by: Andreas Bießmann <andreas.devel@googlemail.com> > > > > --- > > > > Full buildman is running > > > > > > > > > > .... > > > > > > > > > > > +#define BIT(nr) (1UL << (nr)) > > > > > > Why UL? Why not simply 1 << (nr)? > > > > That would give the wrong result for nr == 31 if used as a 64-bit number, > > and > > would produce undefined behavior for nr >= 32 (though even with 1UL that > > would be undefined on 32-bit builds). > > > > > What if I need set ULL bit on 32-bit system? > > > Thanks for explanation. > > > > Yes, ULL would be better. > > That would be BIT_ULL(nr) ? I want to assume that there was some care > given upstream here. It was about 2 years ago now the kernel added a > specific BIT_ULL and family in addition to BIT(nr) from back in 2007. A quick search didn't turn up much justification for keeping them separate (and it seems like using BIT where BIT_ULL is needed could be a source of difficult bugs), but sure, we don't want to encourage writing driver code that will break on Linux. -Scott
Dear Andreas, In message <1441626234-16364-1-git-send-email-andreas.devel@googlemail.com> you wrote: ... > --- a/include/linux/bitops.h > +++ b/include/linux/bitops.h > @@ -3,6 +3,8 @@ > > #include <asm/types.h> > > +#define BIT(nr) (1UL << (nr)) What happens when someone decides to use this on a 64 bit register? Also, this definition is inherently wrong for Power Architecture (TM) systems, where bit 0 is the most significant bit. Best regards, Wolfgang Denk
On 09.09.15 19:22, Scott Wood wrote: > On Tue, 2015-09-08 at 21:01 +0300, ivan.khoronzhuk wrote: >> Hi, Andreas >> >> On 07.09.15 14:43, Andreas Bießmann wrote: >>> From: Heiko Schocher <hs@denx.de> >>> >>> introduce BIT() definition, used in at91_udc gadget >>> driver. >>> >>> Signed-off-by: Heiko Schocher <hs@denx.de> >>> [remove all other occurrences of BIT(x) definition] >>> Signed-off-by: Andreas Bießmann <andreas.devel@googlemail.com> >>> --- >>> Full buildman is running >>> >> >> .... >> >>> >>> +#define BIT(nr) (1UL << (nr)) >> >> Why UL? Why not simply 1 << (nr)? > > That would give the wrong result for nr == 31 if used as a 64-bit number, and Did you mean with 64-bit signed number? After fast glance seems there is no places, but if they are, this can add interesting fixes. > would produce undefined behavior for nr >= 32 (though even with 1UL that > would be undefined on 32-bit builds). > >> What if I need set ULL bit on 32-bit system? >> Thanks for explanation. > > Yes, ULL would be better. > > -Scott > >
On 09.09.15 20:10, Scott Wood wrote: > On Wed, 2015-09-09 at 12:37 -0400, Tom Rini wrote: >> On Wed, Sep 09, 2015 at 11:22:25AM -0500, Scott Wood wrote: >>> On Tue, 2015-09-08 at 21:01 +0300, ivan.khoronzhuk wrote: >>>> Hi, Andreas >>>> >>>> On 07.09.15 14:43, Andreas Bießmann wrote: >>>>> From: Heiko Schocher <hs@denx.de> >>>>> >>>>> introduce BIT() definition, used in at91_udc gadget >>>>> driver. >>>>> >>>>> Signed-off-by: Heiko Schocher <hs@denx.de> >>>>> [remove all other occurrences of BIT(x) definition] >>>>> Signed-off-by: Andreas Bießmann <andreas.devel@googlemail.com> >>>>> --- >>>>> Full buildman is running >>>>> >>>> >>>> .... >>>> >>>>> >>>>> +#define BIT(nr) (1UL << (nr)) >>>> >>>> Why UL? Why not simply 1 << (nr)? >>> >>> That would give the wrong result for nr == 31 if used as a 64-bit number, >>> and >>> would produce undefined behavior for nr >= 32 (though even with 1UL that >>> would be undefined on 32-bit builds). >>> >>>> What if I need set ULL bit on 32-bit system? >>>> Thanks for explanation. >>> >>> Yes, ULL would be better. >> >> That would be BIT_ULL(nr) ? I want to assume that there was some care >> given upstream here. It was about 2 years ago now the kernel added a >> specific BIT_ULL and family in addition to BIT(nr) from back in 2007. > > A quick search didn't turn up much justification for keeping them separate > (and it seems like using BIT where BIT_ULL is needed could be a source of > difficult bugs), but sure, we don't want to encourage writing driver code > that will break on Linux. Better to keep same approach. > > -Scott >
On Wed, 2015-09-09 at 21:52 +0300, Ivan Khoronzhuk wrote: > On 09.09.15 19:22, Scott Wood wrote: > > On Tue, 2015-09-08 at 21:01 +0300, ivan.khoronzhuk wrote: > > > Hi, Andreas > > > > > > On 07.09.15 14:43, Andreas Bießmann wrote: > > > > From: Heiko Schocher <hs@denx.de> > > > > > > > > introduce BIT() definition, used in at91_udc gadget > > > > driver. > > > > > > > > Signed-off-by: Heiko Schocher <hs@denx.de> > > > > [remove all other occurrences of BIT(x) definition] > > > > Signed-off-by: Andreas Bießmann <andreas.devel@googlemail.com> > > > > --- > > > > Full buildman is running > > > > > > > > > > .... > > > > > > > > > > > +#define BIT(nr) (1UL << (nr)) > > > > > > Why UL? Why not simply 1 << (nr)? > > > > That would give the wrong result for nr == 31 if used as a 64-bit number, > > and > Did you mean with 64-bit signed number? No. "u64 x = 1 << 31" will put 0xffffffff80000000 in x, because (unlike 0x80000000 as a constant) "1 << 31" is signed. -Scott
On Wed, 2015-09-09 at 20:25 +0200, Wolfgang Denk wrote: > Dear Andreas, > > In message <1441626234-16364-1-git-send-email-andreas.devel@googlemail.com> > you wrote: > ... > > --- a/include/linux/bitops.h > > +++ b/include/linux/bitops.h > > @@ -3,6 +3,8 @@ > > > > #include <asm/types.h> > > > > +#define BIT(nr) (1UL << (nr)) > > What happens when someone decides to use this on a 64 bit register? > > Also, this definition is inherently wrong for Power Architecture (TM) > systems, where bit 0 is the most significant bit. It's not "inherently wrong" to number bits from LSB on such systems -- it just doesn't match the unusual convention found in PPC documentation and the rotate instructions. In any case, this would be part of the Linux driver compatibility layer rather than the placement in common.h that you previously objected to. -Scott
On Mon, Sep 07, 2015 at 01:43:52PM +0200, Andreas Bießmann wrote: > From: Heiko Schocher <hs@denx.de> > > introduce BIT() definition, used in at91_udc gadget > driver. > > Signed-off-by: Heiko Schocher <hs@denx.de> > [remove all other occurrences of BIT(x) definition] > Signed-off-by: Andreas Bießmann <andreas.devel@googlemail.com> > Acked-by: Stefan Roese <sr@denx.de> > Acked-by: Anatolij Gustschin <agust@denx.de> Applied to u-boot/master, thanks!
diff --git a/arch/arm/include/asm/arch-am33xx/cpu.h b/arch/arm/include/asm/arch-am33xx/cpu.h index 13a9cad..112ac5e 100644 --- a/arch/arm/include/asm/arch-am33xx/cpu.h +++ b/arch/arm/include/asm/arch-am33xx/cpu.h @@ -17,7 +17,6 @@ #include <asm/arch/hardware.h> -#define BIT(x) (1 << x) #define CL_BIT(x) (0 << x) /* Timer register bits */ diff --git a/arch/arm/include/asm/arch-hi6220/gpio.h b/arch/arm/include/asm/arch-hi6220/gpio.h index 98122a2..4fafaef 100644 --- a/arch/arm/include/asm/arch-hi6220/gpio.h +++ b/arch/arm/include/asm/arch-hi6220/gpio.h @@ -11,8 +11,6 @@ #define HI6220_GPIO_BASE(bank) (((bank < 4) ? 0xf8011000 : \ 0xf7020000 - 0x4000) + (0x1000 * bank)) -#define BIT(x) (1 << (x)) - #define HI6220_GPIO_PER_BANK 8 #define HI6220_GPIO_DIR 0x400 diff --git a/arch/arm/include/asm/arch-omap5/cpu.h b/arch/arm/include/asm/arch-omap5/cpu.h index 6109b92..b1513e9 100644 --- a/arch/arm/include/asm/arch-omap5/cpu.h +++ b/arch/arm/include/asm/arch-omap5/cpu.h @@ -56,8 +56,6 @@ struct watchdog { #endif /* __ASSEMBLY__ */ #endif /* __KERNEL_STRICT_NAMES */ -#define BIT(x) (1 << (x)) - #define WD_UNLOCK1 0xAAAA #define WD_UNLOCK2 0x5555 diff --git a/arch/arm/include/asm/arch-tegra/dc.h b/arch/arm/include/asm/arch-tegra/dc.h index 6ffb468..3a87f0b 100644 --- a/arch/arm/include/asm/arch-tegra/dc.h +++ b/arch/arm/include/asm/arch-tegra/dc.h @@ -364,8 +364,6 @@ struct dc_ctlr { struct dc_winbuf_reg winbuf; /* WINBUF A/B/C 0x800 ~ 0x80d */ }; -#define BIT(pos) (1U << pos) - /* DC_CMD_DISPLAY_COMMAND 0x032 */ #define CTRL_MODE_SHIFT 5 #define CTRL_MODE_MASK (0x3 << CTRL_MODE_SHIFT) diff --git a/arch/arm/mach-davinci/cpu.c b/arch/arm/mach-davinci/cpu.c index ff61147..74c3d5d 100644 --- a/arch/arm/mach-davinci/cpu.c +++ b/arch/arm/mach-davinci/cpu.c @@ -28,8 +28,6 @@ DECLARE_GLOBAL_DATA_PTR; #define PLLC_PLLDIV8 0x170 #define PLLC_PLLDIV9 0x174 -#define BIT(x) (1 << (x)) - /* SOC-specific pll info */ #ifdef CONFIG_SOC_DM355 #define ARM_PLLDIV PLLC_PLLDIV1 diff --git a/arch/arm/mach-keystone/include/mach/clock_defs.h b/arch/arm/mach-keystone/include/mach/clock_defs.h index 8ad371f..f8d61d6 100644 --- a/arch/arm/mach-keystone/include/mach/clock_defs.h +++ b/arch/arm/mach-keystone/include/mach/clock_defs.h @@ -11,8 +11,6 @@ #include <asm/arch/hardware.h> -#define BIT(x) (1 << (x)) - /* PLL Control Registers */ struct pllctl_regs { u32 ctl; /* 00 */ diff --git a/arch/arm/mach-keystone/include/mach/hardware.h b/arch/arm/mach-keystone/include/mach/hardware.h index 53f28ec..f98a24e 100644 --- a/arch/arm/mach-keystone/include/mach/hardware.h +++ b/arch/arm/mach-keystone/include/mach/hardware.h @@ -24,8 +24,6 @@ typedef volatile unsigned int *dv_reg_p; #endif -#define BIT(x) (1 << (x)) - #define KS2_DDRPHY_PIR_OFFSET 0x04 #define KS2_DDRPHY_PGCR0_OFFSET 0x08 #define KS2_DDRPHY_PGCR1_OFFSET 0x0C diff --git a/arch/arm/mach-mvebu/include/mach/soc.h b/arch/arm/mach-mvebu/include/mach/soc.h index a8a6b27..02c21bc 100644 --- a/arch/arm/mach-mvebu/include/mach/soc.h +++ b/arch/arm/mach-mvebu/include/mach/soc.h @@ -11,8 +11,6 @@ #ifndef _MVEBU_SOC_H #define _MVEBU_SOC_H -#define BIT(x) (1 << (x)) - #define SOC_MV78460_ID 0x7846 #define SOC_88F6810_ID 0x6810 #define SOC_88F6820_ID 0x6820 diff --git a/arch/arm/mach-zynq/include/mach/gpio.h b/arch/arm/mach-zynq/include/mach/gpio.h index 9e1e7da..0789c49 100644 --- a/arch/arm/mach-zynq/include/mach/gpio.h +++ b/arch/arm/mach-zynq/include/mach/gpio.h @@ -71,6 +71,4 @@ /* GPIO upper 16 bit mask */ #define ZYNQ_GPIO_UPPER_MASK 0xFFFF0000 -#define BIT(x) (1<<x) - #endif /* _ZYNQ_GPIO_H */ diff --git a/drivers/ddr/marvell/a38x/ddr3_init.h b/drivers/ddr/marvell/a38x/ddr3_init.h index e2ff040..cb3fb24 100644 --- a/drivers/ddr/marvell/a38x/ddr3_init.h +++ b/drivers/ddr/marvell/a38x/ddr3_init.h @@ -32,8 +32,6 @@ */ #define MV_DEBUG_INIT -#define BIT(x) (1 << (x)) - #ifdef MV_DEBUG_INIT #define DEBUG_INIT_S(s) puts(s) #define DEBUG_INIT_D(d, l) printf("%x", d) diff --git a/drivers/mtd/nand/jz4740_nand.c b/drivers/mtd/nand/jz4740_nand.c index 7a62cc3..abcedc2 100644 --- a/drivers/mtd/nand/jz4740_nand.c +++ b/drivers/mtd/nand/jz4740_nand.c @@ -16,7 +16,6 @@ #define JZ_NAND_CMD_ADDR (JZ_NAND_DATA_ADDR + 0x8000) #define JZ_NAND_ADDR_ADDR (JZ_NAND_DATA_ADDR + 0x10000) -#define BIT(x) (1 << (x)) #define JZ_NAND_ECC_CTRL_ENCODING BIT(3) #define JZ_NAND_ECC_CTRL_RS BIT(2) #define JZ_NAND_ECC_CTRL_RESET BIT(1) diff --git a/drivers/spi/davinci_spi.c b/drivers/spi/davinci_spi.c index 0a036cc..0bd4f88 100644 --- a/drivers/spi/davinci_spi.c +++ b/drivers/spi/davinci_spi.c @@ -15,8 +15,6 @@ #include <asm/io.h> #include <asm/arch/hardware.h> -#define BIT(x) (1 << (x)) - /* SPIGCR0 */ #define SPIGCR0_SPIENA_MASK 0x1 #define SPIGCR0_SPIRST_MASK 0x0 diff --git a/drivers/spi/ep93xx_spi.c b/drivers/spi/ep93xx_spi.c index 235557e..cb682dd 100644 --- a/drivers/spi/ep93xx_spi.c +++ b/drivers/spi/ep93xx_spi.c @@ -16,8 +16,6 @@ #include <asm/arch/ep93xx.h> - -#define BIT(x) (1<<(x)) #define SSPBASE SPI_BASE #define SSPCR0 0x0000 diff --git a/drivers/video/anx9804.c b/drivers/video/anx9804.c index 83d60d6..37ad69a 100755 --- a/drivers/video/anx9804.c +++ b/drivers/video/anx9804.c @@ -14,8 +14,6 @@ #include <i2c.h> #include "anx9804.h" -#define BIT(x) (1 << (x)) - /* Registers at i2c address 0x38 */ #define ANX9804_HDCP_CONTROL_0_REG 0x01 diff --git a/include/fsl-mc/fsl_mc.h b/include/fsl-mc/fsl_mc.h index 9106f25..9517a4a 100644 --- a/include/fsl-mc/fsl_mc.h +++ b/include/fsl-mc/fsl_mc.h @@ -12,7 +12,6 @@ #define MC_CCSR_BASE_ADDR \ ((struct mc_ccsr_registers __iomem *)0x8340000) -#define BIT(x) (1 << (x)) #define GCR1_P1_STOP BIT(31) #define GCR1_P2_STOP BIT(30) #define GCR1_P1_DE_RST BIT(23) diff --git a/include/linux/bitops.h b/include/linux/bitops.h index e724310..7d30ace 100644 --- a/include/linux/bitops.h +++ b/include/linux/bitops.h @@ -3,6 +3,8 @@ #include <asm/types.h> +#define BIT(nr) (1UL << (nr)) + /* * ffs: find first bit set. This is defined the same way as * the libc and compiler builtin ffs routines, therefore