| Message ID | 20200923083057.113367-1-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: bridge: cdns-mhdp8546: fix compile warning | expand |
On 23/09/2020 11:30, Tomi Valkeinen wrote: > On x64 we get: > > drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c:751:10: warning: conversion from 'long unsigned int' to 'unsigned int' changes value from '18446744073709551613' to '4294967293' [-Woverflow] > > The registers are 32 bit, so fix by casting to u32. > > Fixes: fb43aa0acdfd ("drm: bridge: Add support for Cadence MHDP8546 DPI/DP bridge") > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> I forgot to add Reported-by: Stephen Rothwell <sfr@canb.auug.org.au> Tomi
Hi Tomi, Thank you for the patch. On Wed, Sep 23, 2020 at 11:30:57AM +0300, Tomi Valkeinen wrote: > On x64 we get: > > drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c:751:10: warning: conversion from 'long unsigned int' to 'unsigned int' changes value from '18446744073709551613' to '4294967293' [-Woverflow] > > The registers are 32 bit, so fix by casting to u32. I wonder if we need a BIT32 macro... This is a common issue, it would be nice to handle it in the macros that define register fields. > Fixes: fb43aa0acdfd ("drm: bridge: Add support for Cadence MHDP8546 DPI/DP bridge") > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > index 621ebdbff8a3..d0c65610ebb5 100644 > --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > @@ -748,7 +748,7 @@ static int cdns_mhdp_fw_activate(const struct firmware *fw, > * bridge should already be detached. > */ > if (mhdp->bridge_attached) > - writel(~CDNS_APB_INT_MASK_SW_EVENT_INT, > + writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT, > mhdp->regs + CDNS_APB_INT_MASK); > > spin_unlock(&mhdp->start_lock); > @@ -1689,7 +1689,7 @@ static int cdns_mhdp_attach(struct drm_bridge *bridge, > > /* Enable SW event interrupts */ > if (hw_ready) > - writel(~CDNS_APB_INT_MASK_SW_EVENT_INT, > + writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT, > mhdp->regs + CDNS_APB_INT_MASK); > > return 0; > @@ -2122,7 +2122,7 @@ static void cdns_mhdp_bridge_hpd_enable(struct drm_bridge *bridge) > > /* Enable SW event interrupts */ > if (mhdp->bridge_attached) > - writel(~CDNS_APB_INT_MASK_SW_EVENT_INT, > + writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT, > mhdp->regs + CDNS_APB_INT_MASK); > } >
On 24/09/2020 14:48, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Wed, Sep 23, 2020 at 11:30:57AM +0300, Tomi Valkeinen wrote: >> On x64 we get: >> >> drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c:751:10: warning: conversion from 'long unsigned int' to 'unsigned int' changes value from '18446744073709551613' to '4294967293' [-Woverflow] >> >> The registers are 32 bit, so fix by casting to u32. > > I wonder if we need a BIT32 macro... This is a common issue, it would be > nice to handle it in the macros that define register fields. That's probably a good idea. I think #define BIT32(nr) (1u << (nr)) should work correct on all archs. Although I'm not quite sure if nr should be cast to u32, but in my tests a u64 nr doesn't cause type promotion to u64. Anyway, I'd like to merge this fix as it is to get rid of the warning for the merge window. Tomi
Hi Tomi, Thank you for the patch. > -----Original Message----- > From: Tomi Valkeinen <tomi.valkeinen@ti.com> > Sent: Wednesday, September 23, 2020 2:01 PM > To: dri-devel@lists.freedesktop.org; Swapnil Kashinath Jakhade > <sjakhade@cadence.com>; Yuti Suresh Amonkar <yamonkar@cadence.com> > Cc: Stephen Rothwell <sfr@canb.auug.org.au>; Dave Airlie > <airlied@linux.ie>; Laurent Pinchart <laurent.pinchart@ideasonboard.com>; > Tomi Valkeinen <tomi.valkeinen@ti.com> > Subject: [PATCH] drm: bridge: cdns-mhdp8546: fix compile warning > > EXTERNAL MAIL > > > On x64 we get: > > drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c:751:10: warning: > conversion from 'long unsigned int' to 'unsigned int' changes value from > '18446744073709551613' to '4294967293' [-Woverflow] > > The registers are 32 bit, so fix by casting to u32. > > Fixes: fb43aa0acdfd ("drm: bridge: Add support for Cadence MHDP8546 > DPI/DP bridge") > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > index 621ebdbff8a3..d0c65610ebb5 100644 > --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > @@ -748,7 +748,7 @@ static int cdns_mhdp_fw_activate(const struct > firmware *fw, > * bridge should already be detached. > */ > if (mhdp->bridge_attached) > - writel(~CDNS_APB_INT_MASK_SW_EVENT_INT, > + writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT, > mhdp->regs + CDNS_APB_INT_MASK); > > spin_unlock(&mhdp->start_lock); > @@ -1689,7 +1689,7 @@ static int cdns_mhdp_attach(struct drm_bridge > *bridge, > > /* Enable SW event interrupts */ > if (hw_ready) > - writel(~CDNS_APB_INT_MASK_SW_EVENT_INT, > + writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT, > mhdp->regs + CDNS_APB_INT_MASK); > > return 0; > @@ -2122,7 +2122,7 @@ static void cdns_mhdp_bridge_hpd_enable(struct > drm_bridge *bridge) > > /* Enable SW event interrupts */ > if (mhdp->bridge_attached) > - writel(~CDNS_APB_INT_MASK_SW_EVENT_INT, > + writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT, > mhdp->regs + CDNS_APB_INT_MASK); } > Reviewed-by: Swapnil Jakhade <sjakhade@cadence.com> Thanks, Swapnil > -- > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On Fri, Sep 25, 2020 at 10:36:44AM +0300, Tomi Valkeinen wrote: > On 24/09/2020 14:48, Laurent Pinchart wrote: > > Hi Tomi, > > > > Thank you for the patch. > > > > On Wed, Sep 23, 2020 at 11:30:57AM +0300, Tomi Valkeinen wrote: > >> On x64 we get: > >> > >> drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c:751:10: warning: conversion from 'long unsigned int' to 'unsigned int' changes value from '18446744073709551613' to '4294967293' [-Woverflow] > >> > >> The registers are 32 bit, so fix by casting to u32. > > > > I wonder if we need a BIT32 macro... This is a common issue, it would be > > nice to handle it in the macros that define register fields. > > That's probably a good idea. I think > > #define BIT32(nr) (1u << (nr)) > > should work correct on all archs. Although I'm not quite sure if nr should be cast to u32, but in my > tests a u64 nr doesn't cause type promotion to u64. I don't think we need to support nr values larger than 2^32-1 ;-) > Anyway, I'd like to merge this fix as it is to get rid of the warning for the merge window. Sounds good to me.
On 25/09/2020 15:00, Laurent Pinchart wrote: > On Fri, Sep 25, 2020 at 10:36:44AM +0300, Tomi Valkeinen wrote: >> On 24/09/2020 14:48, Laurent Pinchart wrote: >>> Hi Tomi, >>> >>> Thank you for the patch. >>> >>> On Wed, Sep 23, 2020 at 11:30:57AM +0300, Tomi Valkeinen wrote: >>>> On x64 we get: >>>> >>>> drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c:751:10: warning: conversion from 'long unsigned int' to 'unsigned int' changes value from '18446744073709551613' to '4294967293' [-Woverflow] >>>> >>>> The registers are 32 bit, so fix by casting to u32. >>> >>> I wonder if we need a BIT32 macro... This is a common issue, it would be >>> nice to handle it in the macros that define register fields. >> >> That's probably a good idea. I think >> >> #define BIT32(nr) (1u << (nr)) >> >> should work correct on all archs. Although I'm not quite sure if nr should be cast to u32, but in my >> tests a u64 nr doesn't cause type promotion to u64. > > I don't think we need to support nr values larger than 2^32-1 ;-) That's true, but we might have: u64 nr = 1; BIT32(nr) Afaics, we don't need to cast nr to u32, but maybe that's still the safe thing to do. >> Anyway, I'd like to merge this fix as it is to get rid of the warning for the merge window. > > Sounds good to me. Was that a reviewed-by? =) Tomi
On Fri, Sep 25, 2020 at 03:05:52PM +0300, Tomi Valkeinen wrote: > On 25/09/2020 15:00, Laurent Pinchart wrote: > > On Fri, Sep 25, 2020 at 10:36:44AM +0300, Tomi Valkeinen wrote: > >> On 24/09/2020 14:48, Laurent Pinchart wrote: > >>> Hi Tomi, > >>> > >>> Thank you for the patch. > >>> > >>> On Wed, Sep 23, 2020 at 11:30:57AM +0300, Tomi Valkeinen wrote: > >>>> On x64 we get: > >>>> > >>>> drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c:751:10: warning: conversion from 'long unsigned int' to 'unsigned int' changes value from '18446744073709551613' to '4294967293' [-Woverflow] > >>>> > >>>> The registers are 32 bit, so fix by casting to u32. > >>> > >>> I wonder if we need a BIT32 macro... This is a common issue, it would be > >>> nice to handle it in the macros that define register fields. > >> > >> That's probably a good idea. I think > >> > >> #define BIT32(nr) (1u << (nr)) > >> > >> should work correct on all archs. Although I'm not quite sure if nr should be cast to u32, but in my > >> tests a u64 nr doesn't cause type promotion to u64. > > > > I don't think we need to support nr values larger than 2^32-1 ;-) > > That's true, but we might have: > > u64 nr = 1; > BIT32(nr) > > Afaics, we don't need to cast nr to u32, but maybe that's still the safe thing to do. > > >> Anyway, I'd like to merge this fix as it is to get rid of the warning for the merge window. > > > > Sounds good to me. > > Was that a reviewed-by? =) Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c index 621ebdbff8a3..d0c65610ebb5 100644 --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c @@ -748,7 +748,7 @@ static int cdns_mhdp_fw_activate(const struct firmware *fw, * bridge should already be detached. */ if (mhdp->bridge_attached) - writel(~CDNS_APB_INT_MASK_SW_EVENT_INT, + writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT, mhdp->regs + CDNS_APB_INT_MASK); spin_unlock(&mhdp->start_lock); @@ -1689,7 +1689,7 @@ static int cdns_mhdp_attach(struct drm_bridge *bridge, /* Enable SW event interrupts */ if (hw_ready) - writel(~CDNS_APB_INT_MASK_SW_EVENT_INT, + writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT, mhdp->regs + CDNS_APB_INT_MASK); return 0; @@ -2122,7 +2122,7 @@ static void cdns_mhdp_bridge_hpd_enable(struct drm_bridge *bridge) /* Enable SW event interrupts */ if (mhdp->bridge_attached) - writel(~CDNS_APB_INT_MASK_SW_EVENT_INT, + writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT, mhdp->regs + CDNS_APB_INT_MASK); }
On x64 we get: drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c:751:10: warning: conversion from 'long unsigned int' to 'unsigned int' changes value from '18446744073709551613' to '4294967293' [-Woverflow] The registers are 32 bit, so fix by casting to u32. Fixes: fb43aa0acdfd ("drm: bridge: Add support for Cadence MHDP8546 DPI/DP bridge") Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)