| 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
| ||
|
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