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

[Geert Uytterhoeven <geert@linux-m68k.org>]

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