| Message ID | 20200826202905.11358-1-xypron.glpk@gmx.de |
|---|---|
| State | Accepted, archived |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | [v2,1/1] timer: dw-apb: fix compiler warnings | expand |
On 8/26/20 4:29 PM, Heinrich Schuchardt wrote: > readl() and writel() expect void *. Do not pass an integer value. > > Remove unused include asm/arch/timer.h. > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > Reviewed-by: Bin Meng <bin.meng@windriver.com> > --- > v2: > correct typo in commit message > --- > drivers/timer/dw-apb-timer.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/timer/dw-apb-timer.c b/drivers/timer/dw-apb-timer.c > index 35271b20c8..c8be4417fd 100644 > --- a/drivers/timer/dw-apb-timer.c > +++ b/drivers/timer/dw-apb-timer.c > @@ -14,7 +14,6 @@ > #include <dm/device_compat.h> > > #include <asm/io.h> > -#include <asm/arch/timer.h> > > #define DW_APB_LOAD_VAL 0x0 > #define DW_APB_CURR_VAL 0x4 > @@ -34,7 +33,7 @@ static int dw_apb_timer_get_count(struct udevice *dev, u64 *count) > * requires the count to be incrementing. Invert the > * result. > */ > - *count = timer_conv_64(~readl(priv->regs + DW_APB_CURR_VAL)); > + *count = timer_conv_64(~readl((void *)(priv->regs + DW_APB_CURR_VAL))); How about changing the type of priv.regs to void * and using dev_read_addr_ptr instead of dev_read_addr in probe? > > return 0; > } > @@ -61,8 +60,8 @@ static int dw_apb_timer_probe(struct udevice *dev) > clk_free(&clk); > > /* init timer */ > - writel(0xffffffff, priv->regs + DW_APB_LOAD_VAL); > - writel(0xffffffff, priv->regs + DW_APB_CURR_VAL); > + writel(0xffffffff, (void *)(priv->regs + DW_APB_LOAD_VAL)); > + writel(0xffffffff, (void *)(priv->regs + DW_APB_CURR_VAL)); > setbits_le32(priv->regs + DW_APB_CTRL, 0x3); > > return 0; > -- > 2.27.0 >
Am 26. August 2020 23:13:01 MESZ schrieb Sean Anderson <seanga2@gmail.com>: >On 8/26/20 4:29 PM, Heinrich Schuchardt wrote: >> readl() and writel() expect void *. Do not pass an integer value. >> >> Remove unused include asm/arch/timer.h. >> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >> Reviewed-by: Bin Meng <bin.meng@windriver.com> >> --- >> v2: >> correct typo in commit message >> --- >> drivers/timer/dw-apb-timer.c | 7 +++---- >> 1 file changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/timer/dw-apb-timer.c >b/drivers/timer/dw-apb-timer.c >> index 35271b20c8..c8be4417fd 100644 >> --- a/drivers/timer/dw-apb-timer.c >> +++ b/drivers/timer/dw-apb-timer.c >> @@ -14,7 +14,6 @@ >> #include <dm/device_compat.h> >> >> #include <asm/io.h> >> -#include <asm/arch/timer.h> >> >> #define DW_APB_LOAD_VAL 0x0 >> #define DW_APB_CURR_VAL 0x4 >> @@ -34,7 +33,7 @@ static int dw_apb_timer_get_count(struct udevice >*dev, u64 *count) >> * requires the count to be incrementing. Invert the >> * result. >> */ >> - *count = timer_conv_64(~readl(priv->regs + DW_APB_CURR_VAL)); >> + *count = timer_conv_64(~readl((void *)(priv->regs + >DW_APB_CURR_VAL))); > >How about changing the type of priv.regs to void * and using >dev_read_addr_ptr instead of dev_read_addr in probe? Adding to void* results undefined behavior in C. I know gcc currently supports it, but that does not make it recommendable. If you wanted to change the type, you could use u8*. A cleaner way would be to change regs to a pointer to a structure with all registers and to eliminate the offsets. @Marek Which way do you want it? Best regards Heinrich > >> >> return 0; >> } >> @@ -61,8 +60,8 @@ static int dw_apb_timer_probe(struct udevice *dev) >> clk_free(&clk); >> >> /* init timer */ >> - writel(0xffffffff, priv->regs + DW_APB_LOAD_VAL); >> - writel(0xffffffff, priv->regs + DW_APB_CURR_VAL); >> + writel(0xffffffff, (void *)(priv->regs + DW_APB_LOAD_VAL)); >> + writel(0xffffffff, (void *)(priv->regs + DW_APB_CURR_VAL)); >> setbits_le32(priv->regs + DW_APB_CTRL, 0x3); >> >> return 0; >> -- >> 2.27.0 >>
On 27.08.20 05:35, Heinrich Schuchardt wrote: > Am 26. August 2020 23:13:01 MESZ schrieb Sean Anderson <seanga2@gmail.com>: >> On 8/26/20 4:29 PM, Heinrich Schuchardt wrote: >>> readl() and writel() expect void *. Do not pass an integer value. >>> >>> Remove unused include asm/arch/timer.h. >>> >>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >>> Reviewed-by: Bin Meng <bin.meng@windriver.com> >>> --- >>> v2: >>> correct typo in commit message >>> --- >>> drivers/timer/dw-apb-timer.c | 7 +++---- >>> 1 file changed, 3 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/timer/dw-apb-timer.c >> b/drivers/timer/dw-apb-timer.c >>> index 35271b20c8..c8be4417fd 100644 >>> --- a/drivers/timer/dw-apb-timer.c >>> +++ b/drivers/timer/dw-apb-timer.c >>> @@ -14,7 +14,6 @@ >>> #include <dm/device_compat.h> >>> >>> #include <asm/io.h> >>> -#include <asm/arch/timer.h> >>> >>> #define DW_APB_LOAD_VAL 0x0 >>> #define DW_APB_CURR_VAL 0x4 >>> @@ -34,7 +33,7 @@ static int dw_apb_timer_get_count(struct udevice >> *dev, u64 *count) >>> * requires the count to be incrementing. Invert the >>> * result. >>> */ >>> - *count = timer_conv_64(~readl(priv->regs + DW_APB_CURR_VAL)); >>> + *count = timer_conv_64(~readl((void *)(priv->regs + >> DW_APB_CURR_VAL))); >> >> How about changing the type of priv.regs to void * and using >> dev_read_addr_ptr instead of dev_read_addr in probe? > > Adding to void* results in undefined behavior in C. I know gcc currently supports it, but that does not make it recommendable. > > If you wanted to change the type, you could use u8*. > > A cleaner way would be to change regs to a pointer to a structure with all registers and to eliminate the offsets. > > @Marek > Which way do you want it? Hello Marek, What are your thoughts? Best regards Heinrich > >> >>> >>> return 0; >>> } >>> @@ -61,8 +60,8 @@ static int dw_apb_timer_probe(struct udevice *dev) >>> clk_free(&clk); >>> >>> /* init timer */ >>> - writel(0xffffffff, priv->regs + DW_APB_LOAD_VAL); >>> - writel(0xffffffff, priv->regs + DW_APB_CURR_VAL); >>> + writel(0xffffffff, (void *)(priv->regs + DW_APB_LOAD_VAL)); >>> + writel(0xffffffff, (void *)(priv->regs + DW_APB_CURR_VAL)); >>> setbits_le32(priv->regs + DW_APB_CTRL, 0x3); >>> >>> return 0; >>> -- >>> 2.27.0 >>> >
diff --git a/drivers/timer/dw-apb-timer.c b/drivers/timer/dw-apb-timer.c index 35271b20c8..c8be4417fd 100644 --- a/drivers/timer/dw-apb-timer.c +++ b/drivers/timer/dw-apb-timer.c @@ -14,7 +14,6 @@ #include <dm/device_compat.h> #include <asm/io.h> -#include <asm/arch/timer.h> #define DW_APB_LOAD_VAL 0x0 #define DW_APB_CURR_VAL 0x4 @@ -34,7 +33,7 @@ static int dw_apb_timer_get_count(struct udevice *dev, u64 *count) * requires the count to be incrementing. Invert the * result. */ - *count = timer_conv_64(~readl(priv->regs + DW_APB_CURR_VAL)); + *count = timer_conv_64(~readl((void *)(priv->regs + DW_APB_CURR_VAL))); return 0; } @@ -61,8 +60,8 @@ static int dw_apb_timer_probe(struct udevice *dev) clk_free(&clk); /* init timer */ - writel(0xffffffff, priv->regs + DW_APB_LOAD_VAL); - writel(0xffffffff, priv->regs + DW_APB_CURR_VAL); + writel(0xffffffff, (void *)(priv->regs + DW_APB_LOAD_VAL)); + writel(0xffffffff, (void *)(priv->regs + DW_APB_CURR_VAL)); setbits_le32(priv->regs + DW_APB_CTRL, 0x3); return 0;