| Message ID | 20190309005613.1493-2-marek.vasut@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | [1/2] PCI: rcar: Clean up remaining macros defining bits | expand |
Hi Marek, Thanks for your work. On 2019-03-09 01:56:13 +0100, Marek Vasut wrote: > From: Marek Vasut <marek.vasut+renesas@gmail.com> > > Replace unsigned long with u32 in register accessor functions, > since they access 32bit registers. > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Phil Edworthy <phil.edworthy@renesas.com> > Cc: Simon Horman <horms+renesas@verge.net.au> > Cc: Wolfram Sang <wsa@the-dreams.de> > Cc: linux-renesas-soc@vger.kernel.org > To: linux-pci@vger.kernel.org Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > drivers/pci/controller/pcie-rcar.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c > index 5b8736f0cd6b..1408c8aa758b 100644 > --- a/drivers/pci/controller/pcie-rcar.c > +++ b/drivers/pci/controller/pcie-rcar.c > @@ -152,14 +152,12 @@ struct rcar_pcie { > struct rcar_msi msi; > }; > > -static void rcar_pci_write_reg(struct rcar_pcie *pcie, unsigned long val, > - unsigned long reg) > +static void rcar_pci_write_reg(struct rcar_pcie *pcie, u32 val, u32 reg) > { > writel(val, pcie->base + reg); > } > > -static unsigned long rcar_pci_read_reg(struct rcar_pcie *pcie, > - unsigned long reg) > +static u32 rcar_pci_read_reg(struct rcar_pcie *pcie, u32 reg) > { > return readl(pcie->base + reg); > } > -- > 2.20.1 >
Hi Marek, On Sat, Mar 9, 2019 at 1:56 AM <marek.vasut@gmail.com> wrote: > From: Marek Vasut <marek.vasut+renesas@gmail.com> > > Replace unsigned long with u32 in register accessor functions, > since they access 32bit registers. > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> Thanks for your patch! Minor nit below. Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> BTW, there are a few more suspicious/incorrect uses of unsigned long: - incorrect debug prints on arm64, - more unsigned long register values, - phys_addr_t Care to fix these as well? Thanks! > --- a/drivers/pci/controller/pcie-rcar.c > +++ b/drivers/pci/controller/pcie-rcar.c > @@ -152,14 +152,12 @@ struct rcar_pcie { > struct rcar_msi msi; > }; > > -static void rcar_pci_write_reg(struct rcar_pcie *pcie, unsigned long val, > - unsigned long reg) > +static void rcar_pci_write_reg(struct rcar_pcie *pcie, u32 val, u32 reg) Doesn't unsigned int make more sense for reg? > { > writel(val, pcie->base + reg); > } > > -static unsigned long rcar_pci_read_reg(struct rcar_pcie *pcie, > - unsigned long reg) > +static u32 rcar_pci_read_reg(struct rcar_pcie *pcie, u32 reg) Likewise. Gr{oetje,eeting}s, Geert
On 3/11/19 10:41 AM, Geert Uytterhoeven wrote: > Hi Marek, > > On Sat, Mar 9, 2019 at 1:56 AM <marek.vasut@gmail.com> wrote: >> From: Marek Vasut <marek.vasut+renesas@gmail.com> >> >> Replace unsigned long with u32 in register accessor functions, >> since they access 32bit registers. >> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > > Thanks for your patch! > > Minor nit below. > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > BTW, there are a few more suspicious/incorrect uses of unsigned long: > - incorrect debug prints on arm64, > - more unsigned long register values, > - phys_addr_t > Care to fix these as well? I'll send those shortly, as a separate series since they don't conflict with each other. > Thanks! > >> --- a/drivers/pci/controller/pcie-rcar.c >> +++ b/drivers/pci/controller/pcie-rcar.c >> @@ -152,14 +152,12 @@ struct rcar_pcie { >> struct rcar_msi msi; >> }; >> >> -static void rcar_pci_write_reg(struct rcar_pcie *pcie, unsigned long val, >> - unsigned long reg) >> +static void rcar_pci_write_reg(struct rcar_pcie *pcie, u32 val, u32 reg) > > Doesn't unsigned int make more sense for reg? Isn't u32 more explicit ?
Hi Marek, On Sun, Mar 17, 2019 at 1:06 AM Marek Vasut <marek.vasut@gmail.com> wrote: > On 3/11/19 10:41 AM, Geert Uytterhoeven wrote: > > On Sat, Mar 9, 2019 at 1:56 AM <marek.vasut@gmail.com> wrote: > >> --- a/drivers/pci/controller/pcie-rcar.c > >> +++ b/drivers/pci/controller/pcie-rcar.c > >> @@ -152,14 +152,12 @@ struct rcar_pcie { > >> struct rcar_msi msi; > >> }; > >> > >> -static void rcar_pci_write_reg(struct rcar_pcie *pcie, unsigned long val, > >> - unsigned long reg) > >> +static void rcar_pci_write_reg(struct rcar_pcie *pcie, u32 val, u32 reg) > > > > Doesn't unsigned int make more sense for reg? > > Isn't u32 more explicit ? It's just an offset in the register block, with a range much smaller than u32. Gr{oetje,eeting}s, Geert
On 3/17/19 11:22 AM, Geert Uytterhoeven wrote: > Hi Marek, Hi, > On Sun, Mar 17, 2019 at 1:06 AM Marek Vasut <marek.vasut@gmail.com> wrote: >> On 3/11/19 10:41 AM, Geert Uytterhoeven wrote: >>> On Sat, Mar 9, 2019 at 1:56 AM <marek.vasut@gmail.com> wrote: >>>> --- a/drivers/pci/controller/pcie-rcar.c >>>> +++ b/drivers/pci/controller/pcie-rcar.c >>>> @@ -152,14 +152,12 @@ struct rcar_pcie { >>>> struct rcar_msi msi; >>>> }; >>>> >>>> -static void rcar_pci_write_reg(struct rcar_pcie *pcie, unsigned long val, >>>> - unsigned long reg) >>>> +static void rcar_pci_write_reg(struct rcar_pcie *pcie, u32 val, u32 reg) >>> >>> Doesn't unsigned int make more sense for reg? >> >> Isn't u32 more explicit ? > > It's just an offset in the register block, with a range much smaller than u32. We could use u16 ? However, Bjorn's concern was that using unsigned long for registers was not recommended ; how's unsigned int better ?
Hi Marek, On Mon, Mar 18, 2019 at 12:39 AM Marek Vasut <marek.vasut@gmail.com> wrote: > On 3/17/19 11:22 AM, Geert Uytterhoeven wrote: > > On Sun, Mar 17, 2019 at 1:06 AM Marek Vasut <marek.vasut@gmail.com> wrote: > >> On 3/11/19 10:41 AM, Geert Uytterhoeven wrote: > >>> On Sat, Mar 9, 2019 at 1:56 AM <marek.vasut@gmail.com> wrote: > >>>> --- a/drivers/pci/controller/pcie-rcar.c > >>>> +++ b/drivers/pci/controller/pcie-rcar.c > >>>> @@ -152,14 +152,12 @@ struct rcar_pcie { > >>>> struct rcar_msi msi; > >>>> }; > >>>> > >>>> -static void rcar_pci_write_reg(struct rcar_pcie *pcie, unsigned long val, > >>>> - unsigned long reg) > >>>> +static void rcar_pci_write_reg(struct rcar_pcie *pcie, u32 val, u32 reg) > >>> > >>> Doesn't unsigned int make more sense for reg? > >> > >> Isn't u32 more explicit ? > > > > It's just an offset in the register block, with a range much smaller than u32. > > We could use u16 ? u16 may be more expensive on some processor architectures (MIPS comes too mind, don't know about ARM). > However, Bjorn's concern was that using unsigned long > for registers was not recommended ; Wasn't that comment meant for the size of the register values? > how's unsigned int better ? Basic rule "If you don't care about the size, use (unsigned) int"? Gr{oetje,eeting}s, Geert
On 3/18/19 9:19 AM, Geert Uytterhoeven wrote: > Hi Marek, > > On Mon, Mar 18, 2019 at 12:39 AM Marek Vasut <marek.vasut@gmail.com> wrote: >> On 3/17/19 11:22 AM, Geert Uytterhoeven wrote: >>> On Sun, Mar 17, 2019 at 1:06 AM Marek Vasut <marek.vasut@gmail.com> wrote: >>>> On 3/11/19 10:41 AM, Geert Uytterhoeven wrote: >>>>> On Sat, Mar 9, 2019 at 1:56 AM <marek.vasut@gmail.com> wrote: >>>>>> --- a/drivers/pci/controller/pcie-rcar.c >>>>>> +++ b/drivers/pci/controller/pcie-rcar.c >>>>>> @@ -152,14 +152,12 @@ struct rcar_pcie { >>>>>> struct rcar_msi msi; >>>>>> }; >>>>>> >>>>>> -static void rcar_pci_write_reg(struct rcar_pcie *pcie, unsigned long val, >>>>>> - unsigned long reg) >>>>>> +static void rcar_pci_write_reg(struct rcar_pcie *pcie, u32 val, u32 reg) >>>>> >>>>> Doesn't unsigned int make more sense for reg? >>>> >>>> Isn't u32 more explicit ? >>> >>> It's just an offset in the register block, with a range much smaller than u32. >> >> We could use u16 ? > > u16 may be more expensive on some processor architectures > (MIPS comes too mind, don't know about ARM). On armv8a, none. >> However, Bjorn's concern was that using unsigned long >> for registers was not recommended ; > > Wasn't that comment meant for the size of the register values? > >> how's unsigned int better ? > > Basic rule "If you don't care about the size, use (unsigned) int"? This only applies to the $shift variable, yes ? The rest are u32 since those contain actual values read/written into the registers.
Hi Marek, On Thu, Mar 21, 2019 at 4:20 AM Marek Vasut <marek.vasut@gmail.com> wrote: > On 3/18/19 9:19 AM, Geert Uytterhoeven wrote: > > On Mon, Mar 18, 2019 at 12:39 AM Marek Vasut <marek.vasut@gmail.com> wrote: > >> On 3/17/19 11:22 AM, Geert Uytterhoeven wrote: > >>> On Sun, Mar 17, 2019 at 1:06 AM Marek Vasut <marek.vasut@gmail.com> wrote: > >>>> On 3/11/19 10:41 AM, Geert Uytterhoeven wrote: > >>>>> On Sat, Mar 9, 2019 at 1:56 AM <marek.vasut@gmail.com> wrote: > >>>>>> --- a/drivers/pci/controller/pcie-rcar.c > >>>>>> +++ b/drivers/pci/controller/pcie-rcar.c > >>>>>> @@ -152,14 +152,12 @@ struct rcar_pcie { > >>>>>> struct rcar_msi msi; > >>>>>> }; > >>>>>> > >>>>>> -static void rcar_pci_write_reg(struct rcar_pcie *pcie, unsigned long val, > >>>>>> - unsigned long reg) > >>>>>> +static void rcar_pci_write_reg(struct rcar_pcie *pcie, u32 val, u32 reg) > >>>>> > >>>>> Doesn't unsigned int make more sense for reg? > >>>> > >>>> Isn't u32 more explicit ? > >>> > >>> It's just an offset in the register block, with a range much smaller than u32. > >> > >> We could use u16 ? > > > > u16 may be more expensive on some processor architectures > > (MIPS comes too mind, don't know about ARM). > > On armv8a, none. > > >> However, Bjorn's concern was that using unsigned long > >> for registers was not recommended ; > > > > Wasn't that comment meant for the size of the register values? > > > >> how's unsigned int better ? > > > > Basic rule "If you don't care about the size, use (unsigned) int"? > > This only applies to the $shift variable, yes ? The rest are u32 since > those contain actual values read/written into the registers. There is no "shift" variable. "reg" is a register offset, not an actual register value. Gr{oetje,eeting}s, Geert
On 3/25/19 9:06 AM, Geert Uytterhoeven wrote: > Hi Marek, > > On Thu, Mar 21, 2019 at 4:20 AM Marek Vasut <marek.vasut@gmail.com> wrote: >> On 3/18/19 9:19 AM, Geert Uytterhoeven wrote: >>> On Mon, Mar 18, 2019 at 12:39 AM Marek Vasut <marek.vasut@gmail.com> wrote: >>>> On 3/17/19 11:22 AM, Geert Uytterhoeven wrote: >>>>> On Sun, Mar 17, 2019 at 1:06 AM Marek Vasut <marek.vasut@gmail.com> wrote: >>>>>> On 3/11/19 10:41 AM, Geert Uytterhoeven wrote: >>>>>>> On Sat, Mar 9, 2019 at 1:56 AM <marek.vasut@gmail.com> wrote: >>>>>>>> --- a/drivers/pci/controller/pcie-rcar.c >>>>>>>> +++ b/drivers/pci/controller/pcie-rcar.c >>>>>>>> @@ -152,14 +152,12 @@ struct rcar_pcie { >>>>>>>> struct rcar_msi msi; >>>>>>>> }; >>>>>>>> >>>>>>>> -static void rcar_pci_write_reg(struct rcar_pcie *pcie, unsigned long val, >>>>>>>> - unsigned long reg) >>>>>>>> +static void rcar_pci_write_reg(struct rcar_pcie *pcie, u32 val, u32 reg) >>>>>>> >>>>>>> Doesn't unsigned int make more sense for reg? >>>>>> >>>>>> Isn't u32 more explicit ? >>>>> >>>>> It's just an offset in the register block, with a range much smaller than u32. >>>> >>>> We could use u16 ? >>> >>> u16 may be more expensive on some processor architectures >>> (MIPS comes too mind, don't know about ARM). >> >> On armv8a, none. >> >>>> However, Bjorn's concern was that using unsigned long >>>> for registers was not recommended ; >>> >>> Wasn't that comment meant for the size of the register values? >>> >>>> how's unsigned int better ? >>> >>> Basic rule "If you don't care about the size, use (unsigned) int"? >> >> This only applies to the $shift variable, yes ? The rest are u32 since >> those contain actual values read/written into the registers. > > There is no "shift" variable. "reg" is a register offset, not an actual register > value. Sure
diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c index 5b8736f0cd6b..1408c8aa758b 100644 --- a/drivers/pci/controller/pcie-rcar.c +++ b/drivers/pci/controller/pcie-rcar.c @@ -152,14 +152,12 @@ struct rcar_pcie { struct rcar_msi msi; }; -static void rcar_pci_write_reg(struct rcar_pcie *pcie, unsigned long val, - unsigned long reg) +static void rcar_pci_write_reg(struct rcar_pcie *pcie, u32 val, u32 reg) { writel(val, pcie->base + reg); } -static unsigned long rcar_pci_read_reg(struct rcar_pcie *pcie, - unsigned long reg) +static u32 rcar_pci_read_reg(struct rcar_pcie *pcie, u32 reg) { return readl(pcie->base + reg); }