| Message ID | 20200823050257.564-1-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ti-vpe: cal: Fix compilation on 32-bit ARM | expand |
On Sun, Aug 23, 2020 at 08:02:57AM +0300, Laurent Pinchart wrote: > When compiled on 32-bit ARM, the CAL driver fails with the FIELD_PREP() > macro complaining that the mask is not constant. While all callers of > the inline cal_write_field() function pass a constant mask, the mask > parameter itself is a variable, which likely doesn't please the > compiler. > > Fix it by replacing FIELD_PREP() with a manual implementation. > > Fixes: 50797fb30b95 ("media: ti-vpe: cal: Turn reg_(read|write)_field() into inline functions") > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks! Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
On Mon, Aug 24, 2020 at 12:23:04PM +0300, Sakari Ailus wrote: > On Sun, Aug 23, 2020 at 08:02:57AM +0300, Laurent Pinchart wrote: > > When compiled on 32-bit ARM, the CAL driver fails with the FIELD_PREP() > > macro complaining that the mask is not constant. While all callers of > > the inline cal_write_field() function pass a constant mask, the mask > > parameter itself is a variable, which likely doesn't please the > > compiler. > > > > Fix it by replacing FIELD_PREP() with a manual implementation. > > > > Fixes: 50797fb30b95 ("media: ti-vpe: cal: Turn reg_(read|write)_field() into inline functions") > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Thanks! > > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com> And I forgot to add Reported-by: Sakari Ailus <sakari.ailus@linux.intel.com> Tomi, will you handle this patch ? Could you add the tags ?
On 24/08/2020 13:54, Laurent Pinchart wrote: > On Mon, Aug 24, 2020 at 12:23:04PM +0300, Sakari Ailus wrote: >> On Sun, Aug 23, 2020 at 08:02:57AM +0300, Laurent Pinchart wrote: >>> When compiled on 32-bit ARM, the CAL driver fails with the FIELD_PREP() >>> macro complaining that the mask is not constant. While all callers of >>> the inline cal_write_field() function pass a constant mask, the mask >>> parameter itself is a variable, which likely doesn't please the >>> compiler. >>> >>> Fix it by replacing FIELD_PREP() with a manual implementation. >>> >>> Fixes: 50797fb30b95 ("media: ti-vpe: cal: Turn reg_(read|write)_field() into inline functions") >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> >> Thanks! >> >> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > And I forgot to add > > Reported-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > Tomi, will you handle this patch ? Could you add the tags ? I don't see this when compiling. Which compiler version is it? What's the failing call? If FIELD_PREP is broken, isn't FIELD_GET too (used in cal_read_field)? Tomi
Hi Tomi, On Tue, Aug 25, 2020 at 03:11:24PM +0300, Tomi Valkeinen wrote: > On 24/08/2020 13:54, Laurent Pinchart wrote: > > On Mon, Aug 24, 2020 at 12:23:04PM +0300, Sakari Ailus wrote: > >> On Sun, Aug 23, 2020 at 08:02:57AM +0300, Laurent Pinchart wrote: > >>> When compiled on 32-bit ARM, the CAL driver fails with the FIELD_PREP() > >>> macro complaining that the mask is not constant. While all callers of > >>> the inline cal_write_field() function pass a constant mask, the mask > >>> parameter itself is a variable, which likely doesn't please the > >>> compiler. > >>> > >>> Fix it by replacing FIELD_PREP() with a manual implementation. > >>> > >>> Fixes: 50797fb30b95 ("media: ti-vpe: cal: Turn reg_(read|write)_field() into inline functions") > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> > >> Thanks! > >> > >> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > > And I forgot to add > > > > Reported-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > > Tomi, will you handle this patch ? Could you add the tags ? > > I don't see this when compiling. Which compiler version is it? arm-buildroot-linux-uclibcgnueabihf-gcc.br_real (Buildroot 2020.05.1) 9.3.0 > What's the failing call? CC [M] drivers/media/platform/ti-vpe/cal-camerarx.o In file included from <command-line>: /home/laurent/src/iob/ti/linux/drivers/media/platform/ti-vpe/cal.h: In function ‘cal_write_field’: /home/laurent/src/iob/ti/linux/include/linux/compiler_types.h:319:38: error: call to ‘__compiletime_assert_240’ declared with attribute error: FIELD_PREP: mask is not constant 319 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) | ^ /home/laurent/src/iob/ti/linux/include/linux/compiler_types.h:300:4: note: in definition of macro ‘__compiletime_assert’ 300 | prefix ## suffix(); \ | ^~~~~~ /home/laurent/src/iob/ti/linux/include/linux/compiler_types.h:319:2: note: in expansion of macro ‘_compiletime_assert’ 319 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) | ^~~~~~~~~~~~~~~~~~~ /home/laurent/src/iob/ti/linux/include/linux/build_bug.h:39:37: note: in expansion of macro ‘compiletime_assert’ 39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg) | ^~~~~~~~~~~~~~~~~~ /home/laurent/src/iob/ti/linux/include/linux/bitfield.h:46:3: note: in expansion of macro ‘BUILD_BUG_ON_MSG’ 46 | BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask), \ | ^~~~~~~~~~~~~~~~ /home/laurent/src/iob/ti/linux/include/linux/bitfield.h:94:3: note: in expansion of macro ‘__BF_FIELD_CHECK’ 94 | __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \ | ^~~~~~~~~~~~~~~~ /home/laurent/src/iob/ti/linux/drivers/media/platform/ti-vpe/cal.h:229:9: note: in expansion of macro ‘FIELD_PREP’ 229 | val |= FIELD_PREP(mask, value); | ^~~~~~~~~~ /home/laurent/src/iob/ti/linux/include/linux/compiler_types.h:319:38: error: call to ‘__compiletime_assert_244’ declared with attribute error: BUILD_BUG_ON failed: (((mask) + (1ULL << (__builtin_ffsll(mask) - 1))) & (((mask) + (1ULL << (__builtin_ffsll(mask) - 1))) - 1)) != 0 319 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) | ^ /home/laurent/src/iob/ti/linux/include/linux/compiler_types.h:300:4: note: in definition of macro ‘__compiletime_assert’ 300 | prefix ## suffix(); \ | ^~~~~~ /home/laurent/src/iob/ti/linux/include/linux/compiler_types.h:319:2: note: in expansion of macro ‘_compiletime_assert’ 319 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) | ^~~~~~~~~~~~~~~~~~~ /home/laurent/src/iob/ti/linux/include/linux/build_bug.h:39:37: note: in expansion of macro ‘compiletime_assert’ 39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg) | ^~~~~~~~~~~~~~~~~~ /home/laurent/src/iob/ti/linux/include/linux/build_bug.h:50:2: note: in expansion of macro ‘BUILD_BUG_ON_MSG’ 50 | BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition) | ^~~~~~~~~~~~~~~~ /home/laurent/src/iob/ti/linux/include/linux/build_bug.h:21:2: note: in expansion of macro ‘BUILD_BUG_ON’ 21 | BUILD_BUG_ON(((n) & ((n) - 1)) != 0) | ^~~~~~~~~~~~ /home/laurent/src/iob/ti/linux/include/linux/bitfield.h:54:3: note: in expansion of macro ‘__BUILD_BUG_ON_NOT_POWER_OF_2’ 54 | __BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \ | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /home/laurent/src/iob/ti/linux/include/linux/bitfield.h:94:3: note: in expansion of macro ‘__BF_FIELD_CHECK’ 94 | __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \ | ^~~~~~~~~~~~~~~~ /home/laurent/src/iob/ti/linux/drivers/media/platform/ti-vpe/cal.h:229:9: note: in expansion of macro ‘FIELD_PREP’ 229 | val |= FIELD_PREP(mask, value); | ^~~~~~~~~~ make[5]: *** [/home/laurent/src/iob/ti/linux/scripts/Makefile.build:283: drivers/media/platform/ti-vpe/cal-camerarx.o] Error 1 Kernel defconfig attached, for v5.9-rc1. > If FIELD_PREP is broken, isn't FIELD_GET too (used in cal_read_field)?
On 25/08/2020 15:19, Laurent Pinchart wrote: > Hi Tomi, > > On Tue, Aug 25, 2020 at 03:11:24PM +0300, Tomi Valkeinen wrote: >> On 24/08/2020 13:54, Laurent Pinchart wrote: >>> On Mon, Aug 24, 2020 at 12:23:04PM +0300, Sakari Ailus wrote: >>>> On Sun, Aug 23, 2020 at 08:02:57AM +0300, Laurent Pinchart wrote: >>>>> When compiled on 32-bit ARM, the CAL driver fails with the FIELD_PREP() >>>>> macro complaining that the mask is not constant. While all callers of >>>>> the inline cal_write_field() function pass a constant mask, the mask >>>>> parameter itself is a variable, which likely doesn't please the >>>>> compiler. >>>>> >>>>> Fix it by replacing FIELD_PREP() with a manual implementation. >>>>> >>>>> Fixes: 50797fb30b95 ("media: ti-vpe: cal: Turn reg_(read|write)_field() into inline functions") >>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>>> >>>> Thanks! >>>> >>>> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com> >>> >>> And I forgot to add >>> >>> Reported-by: Sakari Ailus <sakari.ailus@linux.intel.com> >>> >>> Tomi, will you handle this patch ? Could you add the tags ? >> >> I don't see this when compiling. Which compiler version is it? > > arm-buildroot-linux-uclibcgnueabihf-gcc.br_real (Buildroot 2020.05.1) 9.3.0 > Kernel defconfig attached, for v5.9-rc1. Ok. It's something in the defconfig. And identical cal_write_field() call in another function works. I guess it depends on whether the compiler happens to inline or not inline cal_write_field(). I don't see much harm in changing this, so looks good to me. Although I believe cal_read_field() may also start failing at some point, but we can fix it then. Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com> Hans or Mauro, can you pick up this fix for 5.9-rc? Tomi
diff --git a/drivers/media/platform/ti-vpe/cal.h b/drivers/media/platform/ti-vpe/cal.h index e496083715d2..4123405ee0cf 100644 --- a/drivers/media/platform/ti-vpe/cal.h +++ b/drivers/media/platform/ti-vpe/cal.h @@ -226,7 +226,7 @@ static inline void cal_write_field(struct cal_dev *cal, u32 offset, u32 value, u32 val = cal_read(cal, offset); val &= ~mask; - val |= FIELD_PREP(mask, value); + val |= (value << __ffs(mask)) & mask; cal_write(cal, offset, val); }
When compiled on 32-bit ARM, the CAL driver fails with the FIELD_PREP() macro complaining that the mask is not constant. While all callers of the inline cal_write_field() function pass a constant mask, the mask parameter itself is a variable, which likely doesn't please the compiler. Fix it by replacing FIELD_PREP() with a manual implementation. Fixes: 50797fb30b95 ("media: ti-vpe: cal: Turn reg_(read|write)_field() into inline functions") Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/media/platform/ti-vpe/cal.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)