[PATCH 0/3] can: rcar_canfd: Small improvements and cleanups

[PATCH 0/3] can: rcar_canfd: Small improvements and cleanups

[Geert Uytterhoeven]

	Hi all,

This series containssome improvements and cleanups for the R-Car CAN-FD
driver.  It has been tested on R-Car V4H (White Hawk and White Hawk
Single).

Thanks for your comments!

Geert Uytterhoeven (3):
  can: rcar_canfd: Simplify clock handling
  can: rcar_canfd: Improve printing of global operational state
  can: rcar_canfd: Remove superfluous parentheses in address
    calculations

 drivers/net/can/rcar/rcar_canfd.c | 41 ++++++++++++-------------------
 1 file changed, 16 insertions(+), 25 deletions(-)

-- 
2.34.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH 1/3] can: rcar_canfd: Simplify clock handling

[Geert Uytterhoeven]

The main CAN clock is either the internal CANFD clock, or the external
CAN clock.  Hence replace the two-valued enum by a simple boolean flag.
Consolidate all CANFD clock handling inside a single branch.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/net/can/rcar/rcar_canfd.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index b828427187353d6f..474840b58e8f13f1 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -508,12 +508,6 @@
  */
 #define RCANFD_CFFIFO_IDX		0
 
-/* fCAN clock select register settings */
-enum rcar_canfd_fcanclk {
-	RCANFD_CANFDCLK = 0,		/* CANFD clock */
-	RCANFD_EXTCLK,			/* Externally input clock */
-};
-
 struct rcar_canfd_global;
 
 struct rcar_canfd_hw_info {
@@ -545,8 +539,8 @@ struct rcar_canfd_global {
 	struct platform_device *pdev;	/* Respective platform device */
 	struct clk *clkp;		/* Peripheral clock */
 	struct clk *can_clk;		/* fCAN clock */
-	enum rcar_canfd_fcanclk fcan;	/* CANFD or Ext clock */
 	unsigned long channels_mask;	/* Enabled channels mask */
+	bool extclk;			/* CANFD or Ext clock */
 	bool fdmode;			/* CAN FD or Classical CAN only mode */
 	struct reset_control *rstc1;
 	struct reset_control *rstc2;
@@ -777,7 +771,7 @@ static void rcar_canfd_configure_controller(struct rcar_canfd_global *gpriv)
 		cfg |= RCANFD_GCFG_CMPOC;
 
 	/* Set External Clock if selected */
-	if (gpriv->fcan != RCANFD_CANFDCLK)
+	if (gpriv->extclk)
 		cfg |= RCANFD_GCFG_DCS;
 
 	rcar_canfd_set_bit(gpriv->base, RCANFD_GCFG, cfg);
@@ -1941,16 +1935,12 @@ static int rcar_canfd_probe(struct platform_device *pdev)
 			return dev_err_probe(dev, PTR_ERR(gpriv->can_clk),
 					     "cannot get canfd clock\n");
 
-		gpriv->fcan = RCANFD_CANFDCLK;
-
+		/* CANFD clock may be further divided within the IP */
+		fcan_freq = clk_get_rate(gpriv->can_clk) / info->postdiv;
 	} else {
-		gpriv->fcan = RCANFD_EXTCLK;
+		fcan_freq = clk_get_rate(gpriv->can_clk);
+		gpriv->extclk = true;
 	}
-	fcan_freq = clk_get_rate(gpriv->can_clk);
-
-	if (gpriv->fcan == RCANFD_CANFDCLK)
-		/* CANFD clock is further divided by (1/2) within the IP */
-		fcan_freq /= info->postdiv;
 
 	addr = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(addr)) {
@@ -2060,7 +2050,7 @@ static int rcar_canfd_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, gpriv);
 	dev_info(dev, "global operational state (clk %d, fdmode %d)\n",
-		 gpriv->fcan, gpriv->fdmode);
+		 gpriv->extclk, gpriv->fdmode);
 	return 0;
 
 fail_channel:
-- 
2.34.1

Re: [PATCH 1/3] can: rcar_canfd: Simplify clock handling

[Vincent MAILHOL]

On Wed. 29 May 2024 at 18:12, Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
> The main CAN clock is either the internal CANFD clock, or the external
> CAN clock.  Hence replace the two-valued enum by a simple boolean flag.
> Consolidate all CANFD clock handling inside a single branch.

For what it is worth, your patch also saves up to 8 bytes in struct
rcar_canfd_global (depends on the architecture).

Before:

  $ pahole drivers/net/can/rcar/rcar_canfd.o -C rcar_canfd_global
  struct rcar_canfd_global {
      struct rcar_canfd_channel * ch[8];               /*     0    64 */
      /* --- cacheline 1 boundary (64 bytes) --- */
      void *                     base;                 /*    64     8 */
      struct platform_device *   pdev;                 /*    72     8 */
      struct clk *               clkp;                 /*    80     8 */
      struct clk *               can_clk;              /*    88     8 */
      enum rcar_canfd_fcanclk    fcan;                 /*    96     4 */

      /* XXX 4 bytes hole, try to pack */

      long unsigned int          channels_mask;        /*   104     8 */
      bool                       fdmode;               /*   112     1 */

      /* XXX 7 bytes hole, try to pack */

      struct reset_control *     rstc1;                /*   120     8 */
      /* --- cacheline 2 boundary (128 bytes) --- */
      struct reset_control *     rstc2;                /*   128     8 */
      const struct rcar_canfd_hw_info  * info;         /*   136     8 */

      /* size: 144, cachelines: 3, members: 11 */
      /* sum members: 133, holes: 2, sum holes: 11 */
      /* last cacheline: 16 bytes */
  };

After:

  $ pahole drivers/net/can/rcar/rcar_canfd.o -C rcar_canfd_global
  struct rcar_canfd_global {
      struct rcar_canfd_channel * ch[8];               /*     0    64 */
      /* --- cacheline 1 boundary (64 bytes) --- */
      void *                     base;                 /*    64     8 */
      struct platform_device *   pdev;                 /*    72     8 */
      struct clk *               clkp;                 /*    80     8 */
      struct clk *               can_clk;              /*    88     8 */
      long unsigned int          channels_mask;        /*    96     8 */
      bool                       extclk;               /*   104     1 */
      bool                       fdmode;               /*   105     1 */

      /* XXX 6 bytes hole, try to pack */

      struct reset_control *     rstc1;                /*   112     8 */
      struct reset_control *     rstc2;                /*   120     8 */
      /* --- cacheline 2 boundary (128 bytes) --- */
      const struct rcar_canfd_hw_info  * info;         /*   128     8 */

      /* size: 136, cachelines: 3, members: 11 */
      /* sum members: 130, holes: 1, sum holes: 6 */
      /* last cacheline: 8 bytes */
  };

> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/net/can/rcar/rcar_canfd.c | 24 +++++++-----------------
>  1 file changed, 7 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> index b828427187353d6f..474840b58e8f13f1 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c
> @@ -508,12 +508,6 @@
>   */
>  #define RCANFD_CFFIFO_IDX              0
>
> -/* fCAN clock select register settings */
> -enum rcar_canfd_fcanclk {
> -       RCANFD_CANFDCLK = 0,            /* CANFD clock */
> -       RCANFD_EXTCLK,                  /* Externally input clock */
> -};
> -
>  struct rcar_canfd_global;
>
>  struct rcar_canfd_hw_info {
> @@ -545,8 +539,8 @@ struct rcar_canfd_global {
>         struct platform_device *pdev;   /* Respective platform device */
>         struct clk *clkp;               /* Peripheral clock */
>         struct clk *can_clk;            /* fCAN clock */
> -       enum rcar_canfd_fcanclk fcan;   /* CANFD or Ext clock */
>         unsigned long channels_mask;    /* Enabled channels mask */
> +       bool extclk;                    /* CANFD or Ext clock */
>         bool fdmode;                    /* CAN FD or Classical CAN only mode */

Notwithstanding comment: you may consider to replace those two booleans by a:

          unsigned int flags;

This way, no more fields would be needed in the future if more quirks are added.

>         struct reset_control *rstc1;
>         struct reset_control *rstc2;
> @@ -777,7 +771,7 @@ static void rcar_canfd_configure_controller(struct rcar_canfd_global *gpriv)
>                 cfg |= RCANFD_GCFG_CMPOC;
>
>         /* Set External Clock if selected */
> -       if (gpriv->fcan != RCANFD_CANFDCLK)
> +       if (gpriv->extclk)
>                 cfg |= RCANFD_GCFG_DCS;
>
>         rcar_canfd_set_bit(gpriv->base, RCANFD_GCFG, cfg);
> @@ -1941,16 +1935,12 @@ static int rcar_canfd_probe(struct platform_device *pdev)
>                         return dev_err_probe(dev, PTR_ERR(gpriv->can_clk),
>                                              "cannot get canfd clock\n");
>
> -               gpriv->fcan = RCANFD_CANFDCLK;
> -
> +               /* CANFD clock may be further divided within the IP */
> +               fcan_freq = clk_get_rate(gpriv->can_clk) / info->postdiv;
>         } else {
> -               gpriv->fcan = RCANFD_EXTCLK;
> +               fcan_freq = clk_get_rate(gpriv->can_clk);
> +               gpriv->extclk = true;
>         }
> -       fcan_freq = clk_get_rate(gpriv->can_clk);
> -
> -       if (gpriv->fcan == RCANFD_CANFDCLK)
> -               /* CANFD clock is further divided by (1/2) within the IP */
> -               fcan_freq /= info->postdiv;
>
>         addr = devm_platform_ioremap_resource(pdev, 0);
>         if (IS_ERR(addr)) {
> @@ -2060,7 +2050,7 @@ static int rcar_canfd_probe(struct platform_device *pdev)
>
>         platform_set_drvdata(pdev, gpriv);
>         dev_info(dev, "global operational state (clk %d, fdmode %d)\n",
> -                gpriv->fcan, gpriv->fdmode);
> +                gpriv->extclk, gpriv->fdmode);
>         return 0;
>
>  fail_channel:
> --
> 2.34.1
>
>

Re: [PATCH 1/3] can: rcar_canfd: Simplify clock handling

[Geert Uytterhoeven]

Hi Vincent,

On Sun, Jun 2, 2024 at 10:03 AM Vincent MAILHOL
<mailhol.vincent@wanadoo.fr> wrote:
> On Wed. 29 May 2024 at 18:12, Geert Uytterhoeven
> <geert+renesas@glider.be> wrote:
> > The main CAN clock is either the internal CANFD clock, or the external
> > CAN clock.  Hence replace the two-valued enum by a simple boolean flag.
> > Consolidate all CANFD clock handling inside a single branch.
>
> For what it is worth, your patch also saves up to 8 bytes in struct
> rcar_canfd_global (depends on the architecture).

True.

> > @@ -545,8 +539,8 @@ struct rcar_canfd_global {
> >         struct platform_device *pdev;   /* Respective platform device */
> >         struct clk *clkp;               /* Peripheral clock */
> >         struct clk *can_clk;            /* fCAN clock */
> > -       enum rcar_canfd_fcanclk fcan;   /* CANFD or Ext clock */
> >         unsigned long channels_mask;    /* Enabled channels mask */
> > +       bool extclk;                    /* CANFD or Ext clock */
> >         bool fdmode;                    /* CAN FD or Classical CAN only mode */
>
> Notwithstanding comment: you may consider to replace those two booleans by a:
>
>           unsigned int flags;
>
> This way, no more fields would be needed in the future if more quirks are added.

Using "unsigned int flags" and BIT(x) flags would increase code size
by 8 bytes (arm/arm64).
Using "unsigned int foo:1" bitfields would increase code size by 16
(arm) or 12 (arm64) bytes.
So as long as we can fit more bools inside the hole, it is more
efficient to do so...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/3] can: rcar_canfd: Simplify clock handling

[Vincent MAILHOL]

On Thu. 6 June 2024 at 19:15, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Vincent,
>
> On Sun, Jun 2, 2024 at 10:03 AM Vincent MAILHOL
> <mailhol.vincent@wanadoo.fr> wrote:
> > On Wed. 29 May 2024 at 18:12, Geert Uytterhoeven
> > <geert+renesas@glider.be> wrote:
> > > The main CAN clock is either the internal CANFD clock, or the external
> > > CAN clock.  Hence replace the two-valued enum by a simple boolean flag.
> > > Consolidate all CANFD clock handling inside a single branch.
> >
> > For what it is worth, your patch also saves up to 8 bytes in struct
> > rcar_canfd_global (depends on the architecture).
>
> True.
>
> > > @@ -545,8 +539,8 @@ struct rcar_canfd_global {
> > >         struct platform_device *pdev;   /* Respective platform device */
> > >         struct clk *clkp;               /* Peripheral clock */
> > >         struct clk *can_clk;            /* fCAN clock */
> > > -       enum rcar_canfd_fcanclk fcan;   /* CANFD or Ext clock */
> > >         unsigned long channels_mask;    /* Enabled channels mask */
> > > +       bool extclk;                    /* CANFD or Ext clock */
> > >         bool fdmode;                    /* CAN FD or Classical CAN only mode */
> >
> > Notwithstanding comment: you may consider to replace those two booleans by a:
> >
> >           unsigned int flags;
> >
> > This way, no more fields would be needed in the future if more quirks are added.
>
> Using "unsigned int flags" and BIT(x) flags would increase code size
> by 8 bytes (arm/arm64).

I am not sure where you derive your figure from, but looking at the pahole:

  $ pahole drivers/net/can/rcar/rcar_canfd.o -C rcar_canfd_global
  struct rcar_canfd_global {
      struct rcar_canfd_channel * ch[8];               /*     0    64 */
      /* --- cacheline 1 boundary (64 bytes) --- */
      void *                     base;                 /*    64     8 */
      struct platform_device *   pdev;                 /*    72     8 */
      struct clk *               clkp;                 /*    80     8 */
      struct clk *               can_clk;              /*    88     8 */
      long unsigned int          channels_mask;        /*    96     8 */
      bool                       extclk;               /*   104     1 */
      bool                       fdmode;               /*   105     1 */

      /* XXX 6 bytes hole, try to pack */

      struct reset_control *     rstc1;                /*   112     8 */
      struct reset_control *     rstc2;                /*   120     8 */
      /* --- cacheline 2 boundary (128 bytes) --- */
      const struct rcar_canfd_hw_info  * info;         /*   128     8 */

      /* size: 136, cachelines: 3, members: 11 */
      /* sum members: 130, holes: 1, sum holes: 6 */
      /* last cacheline: 8 bytes */
  };

on a 64 bits architecture, you are currently using two booleans (two
bytes), followed by a hole of six bytes, which is a total of eight
bytes. This should be way enough to fit an unsigned int. Same would go
on 32 bits architecture in which you would use two bytes for the
booleans and have a hole of two bytes, which is four bytes: one more
time enough for an unsigned int.

In both scenarios, you are not consuming more bytes, nor are you
saving bytes. It is a neutral change.

Of course, the pahole above was done on x86_64, but as far as I know,
arm and arm64 paddings behave similarly.

> Using "unsigned int foo:1" bitfields would increase code size by 16
> (arm) or 12 (arm64) bytes.
> So as long as we can fit more bools inside the hole, it is more
> efficient to do so...

I do not get this either. Where did you get your 16 bytes from? If I do:

  struct foo {
          unsigned int foo1:1;
          unsigned int foo2:1;
  };

and

  struct bar {
          unsigned int flags;
  };

then I am pretty sure that sizeof(struct foo) is the same as
sizeof(struct bar). That's the point of the bitfields: as long as the
total of the bitfields fit in the type, the total size consumed by the
bitfield is the same as the type size.

But just to reiter my previous message, these are notwithstanding
comments. I am fine if you keep the patch as-is ;)

> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
>

Re: [PATCH 1/3] can: rcar_canfd: Simplify clock handling

[Geert Uytterhoeven]

Hi Vincent,

On Thu, Jun 6, 2024 at 1:05 PM Vincent MAILHOL
<mailhol.vincent@wanadoo.fr> wrote:
> On Thu. 6 June 2024 at 19:15, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Sun, Jun 2, 2024 at 10:03 AM Vincent MAILHOL
> > <mailhol.vincent@wanadoo.fr> wrote:
> > > On Wed. 29 May 2024 at 18:12, Geert Uytterhoeven
> > > <geert+renesas@glider.be> wrote:
> > > > The main CAN clock is either the internal CANFD clock, or the external
> > > > CAN clock.  Hence replace the two-valued enum by a simple boolean flag.
> > > > Consolidate all CANFD clock handling inside a single branch.
> > >
> > > For what it is worth, your patch also saves up to 8 bytes in struct
> > > rcar_canfd_global (depends on the architecture).
> >
> > True.
> >
> > > > @@ -545,8 +539,8 @@ struct rcar_canfd_global {
> > > >         struct platform_device *pdev;   /* Respective platform device */
> > > >         struct clk *clkp;               /* Peripheral clock */
> > > >         struct clk *can_clk;            /* fCAN clock */
> > > > -       enum rcar_canfd_fcanclk fcan;   /* CANFD or Ext clock */
> > > >         unsigned long channels_mask;    /* Enabled channels mask */
> > > > +       bool extclk;                    /* CANFD or Ext clock */
> > > >         bool fdmode;                    /* CAN FD or Classical CAN only mode */
> > >
> > > Notwithstanding comment: you may consider to replace those two booleans by a:
> > >
> > >           unsigned int flags;
> > >
> > > This way, no more fields would be needed in the future if more quirks are added.
> >
> > Using "unsigned int flags" and BIT(x) flags would increase code size
> > by 8 bytes (arm/arm64).
>
> I am not sure where you derive your figure from, but looking at the pahole:

pahole shows the size of data structures.

> > Using "unsigned int foo:1" bitfields would increase code size by 16
> > (arm) or 12 (arm64) bytes.
> > So as long as we can fit more bools inside the hole, it is more
> > efficient to do so...
>
> I do not get this either. Where did you get your 16 bytes from? If I do:

I also looked at code size[*]: while storing bits takes less space than
storing bytes, processing bits may require more instructions than
processing bytes (depending on the architecture).

[*] size drivers/net/can/rcar/rcar_canfd.o

> But just to reiter my previous message, these are notwithstanding
> comments. I am fine if you keep the patch as-is ;)

So I'd like to keep the patch as-is.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/3] can: rcar_canfd: Simplify clock handling

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2063 bytes --]

On 06.06.2024 13:38:24, Geert Uytterhoeven wrote:
> > > > > @@ -545,8 +539,8 @@ struct rcar_canfd_global {
> > > > >         struct platform_device *pdev;   /* Respective platform device */
> > > > >         struct clk *clkp;               /* Peripheral clock */
> > > > >         struct clk *can_clk;            /* fCAN clock */
> > > > > -       enum rcar_canfd_fcanclk fcan;   /* CANFD or Ext clock */
> > > > >         unsigned long channels_mask;    /* Enabled channels mask */
> > > > > +       bool extclk;                    /* CANFD or Ext clock */
> > > > >         bool fdmode;                    /* CAN FD or Classical CAN only mode */
> > > >
> > > > Notwithstanding comment: you may consider to replace those two booleans by a:
> > > >
> > > >           unsigned int flags;
> > > >
> > > > This way, no more fields would be needed in the future if more quirks are added.
> > >
> > > Using "unsigned int flags" and BIT(x) flags would increase code size
> > > by 8 bytes (arm/arm64).
> >
> > I am not sure where you derive your figure from, but looking at the pahole:
> 
> pahole shows the size of data structures.
> 
> > > Using "unsigned int foo:1" bitfields would increase code size by 16
> > > (arm) or 12 (arm64) bytes.
> > > So as long as we can fit more bools inside the hole, it is more
> > > efficient to do so...
> >
> > I do not get this either. Where did you get your 16 bytes from? If I do:
> 
> I also looked at code size[*]: while storing bits takes less space than
> storing bytes, processing bits may require more instructions than
> processing bytes (depending on the architecture).
> 
> [*] size drivers/net/can/rcar/rcar_canfd.o

You have probably used "scripts/bloat-o-meter" from the kernel source
for this, right?

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] can: rcar_canfd: Simplify clock handling

[Geert Uytterhoeven]

Hi Marc,

On Fri, Jun 21, 2024 at 10:36 AM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 06.06.2024 13:38:24, Geert Uytterhoeven wrote:
> > > > > > @@ -545,8 +539,8 @@ struct rcar_canfd_global {
> > > > > >         struct platform_device *pdev;   /* Respective platform device */
> > > > > >         struct clk *clkp;               /* Peripheral clock */
> > > > > >         struct clk *can_clk;            /* fCAN clock */
> > > > > > -       enum rcar_canfd_fcanclk fcan;   /* CANFD or Ext clock */
> > > > > >         unsigned long channels_mask;    /* Enabled channels mask */
> > > > > > +       bool extclk;                    /* CANFD or Ext clock */
> > > > > >         bool fdmode;                    /* CAN FD or Classical CAN only mode */
> > > > >
> > > > > Notwithstanding comment: you may consider to replace those two booleans by a:
> > > > >
> > > > >           unsigned int flags;
> > > > >
> > > > > This way, no more fields would be needed in the future if more quirks are added.
> > > >
> > > > Using "unsigned int flags" and BIT(x) flags would increase code size
> > > > by 8 bytes (arm/arm64).
> > >
> > > I am not sure where you derive your figure from, but looking at the pahole:
> >
> > pahole shows the size of data structures.
> >
> > > > Using "unsigned int foo:1" bitfields would increase code size by 16
> > > > (arm) or 12 (arm64) bytes.
> > > > So as long as we can fit more bools inside the hole, it is more
> > > > efficient to do so...
> > >
> > > I do not get this either. Where did you get your 16 bytes from? If I do:
> >
> > I also looked at code size[*]: while storing bits takes less space than
> > storing bytes, processing bits may require more instructions than
> > processing bytes (depending on the architecture).
> >
> > [*] size drivers/net/can/rcar/rcar_canfd.o
>
> You have probably used "scripts/bloat-o-meter" from the kernel source
> for this, right?

Not this time; I used "size" from the binutils package instead.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/3] can: rcar_canfd: Simplify clock handling

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 439 bytes --]

On Wed, May 29, 2024 at 11:12:13AM +0200, Geert Uytterhoeven wrote:
> The main CAN clock is either the internal CANFD clock, or the external
> CAN clock.  Hence replace the two-valued enum by a simple boolean flag.
> Consolidate all CANFD clock handling inside a single branch.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Nice simplification!

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/3] can: rcar_canfd: Improve printing of global operational state

[Geert Uytterhoeven]

Replace the printing of internal numerical values by the printing of
strings reflecting their meaning, to make the message self-explanatory.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/net/can/rcar/rcar_canfd.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index 474840b58e8f13f1..c2c1c47bcc7a166c 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -2049,8 +2049,9 @@ static int rcar_canfd_probe(struct platform_device *pdev)
 	}
 
 	platform_set_drvdata(pdev, gpriv);
-	dev_info(dev, "global operational state (clk %d, fdmode %d)\n",
-		 gpriv->extclk, gpriv->fdmode);
+	dev_info(dev, "global operational state (%s clk, %s mode)\n",
+		 gpriv->extclk ? "ext" : "canfd",
+		 gpriv->fdmode ? "fd" : "classical");
 	return 0;
 
 fail_channel:
-- 
2.34.1

Re: [PATCH 2/3] can: rcar_canfd: Improve printing of global operational state

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 347 bytes --]

On Wed, May 29, 2024 at 11:12:14AM +0200, Geert Uytterhoeven wrote:
> Replace the printing of internal numerical values by the printing of
> strings reflecting their meaning, to make the message self-explanatory.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Yes, useful!

Reviewed-by: Wolfram Sang <wsa@kernel.org>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 3/3] can: rcar_canfd: Remove superfluous parentheses in address calculations

[Geert Uytterhoeven]

There is no need to wrap simple variables or multiplications inside
parentheses.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/net/can/rcar/rcar_canfd.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index c2c1c47bcc7a166c..c919668bbe7a5541 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -627,28 +627,28 @@ static inline void rcar_canfd_update(u32 mask, u32 val, u32 __iomem *reg)
 
 static inline u32 rcar_canfd_read(void __iomem *base, u32 offset)
 {
-	return readl(base + (offset));
+	return readl(base + offset);
 }
 
 static inline void rcar_canfd_write(void __iomem *base, u32 offset, u32 val)
 {
-	writel(val, base + (offset));
+	writel(val, base + offset);
 }
 
 static void rcar_canfd_set_bit(void __iomem *base, u32 reg, u32 val)
 {
-	rcar_canfd_update(val, val, base + (reg));
+	rcar_canfd_update(val, val, base + reg);
 }
 
 static void rcar_canfd_clear_bit(void __iomem *base, u32 reg, u32 val)
 {
-	rcar_canfd_update(val, 0, base + (reg));
+	rcar_canfd_update(val, 0, base + reg);
 }
 
 static void rcar_canfd_update_bit(void __iomem *base, u32 reg,
 				  u32 mask, u32 val)
 {
-	rcar_canfd_update(mask, val, base + (reg));
+	rcar_canfd_update(mask, val, base + reg);
 }
 
 static void rcar_canfd_get_data(struct rcar_canfd_channel *priv,
@@ -659,7 +659,7 @@ static void rcar_canfd_get_data(struct rcar_canfd_channel *priv,
 	lwords = DIV_ROUND_UP(cf->len, sizeof(u32));
 	for (i = 0; i < lwords; i++)
 		*((u32 *)cf->data + i) =
-			rcar_canfd_read(priv->base, off + (i * sizeof(u32)));
+			rcar_canfd_read(priv->base, off + i * sizeof(u32));
 }
 
 static void rcar_canfd_put_data(struct rcar_canfd_channel *priv,
@@ -669,7 +669,7 @@ static void rcar_canfd_put_data(struct rcar_canfd_channel *priv,
 
 	lwords = DIV_ROUND_UP(cf->len, sizeof(u32));
 	for (i = 0; i < lwords; i++)
-		rcar_canfd_write(priv->base, off + (i * sizeof(u32)),
+		rcar_canfd_write(priv->base, off + i * sizeof(u32),
 				 *((u32 *)cf->data + i));
 }
 
-- 
2.34.1

Re: [PATCH 3/3] can: rcar_canfd: Remove superfluous parentheses in address calculations

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 289 bytes --]

On Wed, May 29, 2024 at 11:12:15AM +0200, Geert Uytterhoeven wrote:
> There is no need to wrap simple variables or multiplications inside
> parentheses.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/3] can: rcar_canfd: Small improvements and cleanups

[Vincent MAILHOL]

On Wed. 29 mai 2024 at 18:12, Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
>         Hi all,
>
> This series containssome improvements and cleanups for the R-Car CAN-FD
> driver.  It has been tested on R-Car V4H (White Hawk and White Hawk
> Single).
>
> Thanks for your comments!

I left one notwithstanding comment on the first patch. With or without
that nitpick addressed, and for the full series:

Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

Re: [PATCH 0/3] can: rcar_canfd: Small improvements and cleanups

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 643 bytes --]

On 29.05.2024 11:12:12, Geert Uytterhoeven wrote:
> This series containssome improvements and cleanups for the R-Car CAN-FD
                    ^^^
I've added the missing space while merging this series.

> driver.  It has been tested on R-Car V4H (White Hawk and White Hawk
> Single).

Applied to linux-can-next. I'll include this in the next PR.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]