lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAMZ6RqKZdo1Mk=tY-vqCm0YYr_Qk8m53+LHXqeM+1LL=S=+RqQ@mail.gmail.com>
Date: Sun, 2 Jun 2024 17:03:02 +0900
From: Vincent MAILHOL <mailhol.vincent@...adoo.fr>
To: Geert Uytterhoeven <geert+renesas@...der.be>
Cc: Marc Kleine-Budde <mkl@...gutronix.de>, Wolfram Sang <wsa+renesas@...g-engineering.com>, 
	linux-can@...r.kernel.org, linux-renesas-soc@...r.kernel.org, 
	netdev@...r.kernel.org
Subject: Re: [PATCH 1/3] can: rcar_canfd: Simplify clock handling

On Wed. 29 May 2024 at 18:12, Geert Uytterhoeven
<geert+renesas@...der.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@...der.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
>
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ