Re: [PATCH RFC] pinctrl: sh-pfc: pfc-r8a77965: Optimize pinctrl image size for RZ/G2N

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Biju,

On Wed, Oct 14, 2020 at 1:02 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> Optimize pinctrl image size, when only RZ/G2N is enabled in the defconfig.
> (ie, disabling CONFIG_ARCH_R8A77965 from the defconfig)
>
> with this patch and disabling CONFIG_ARCH_R8A77965:
> $ size drivers/pinctrl/sh-pfc/pfc-r8a77965.o
>    text    data     bss     dec     hex filename
>   49384       0       0   49384    c0e8 drivers/pinctrl/sh-pfc/pfc-r8a77965.o
>
> without patch:
> $ size drivers/pinctrl/sh-pfc/pfc-r8a77965.o
>    text    data     bss     dec     hex filename
>   51848       0       0   51848    ca88 drivers/pinctrl/sh-pfc/pfc-r8a77965.o
>
> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>

Thanks for your patch!

> ---
> This patch will save ~ 6KB=(3x 2KB/SoC) of memory on RZ/G2[HMN] u-boot[1] with
> multi dtb support. As per discussion [1], u-boot imports PFC and Clock tables from Linux.
>
> [1] https://patchwork.ozlabs.org/project/uboot/patch/20201013085205.6075-4-biju.das.jz@xxxxxxxxxxxxxx/
>
> 1) By compiling out Automative parts
> $ size drivers/pinctrl/renesas/pfc-r8a77965.o
>    text    data     bss     dec     hex filename
>   46141       0       0   46141    b43d drivers/pinctrl/renesas/pfc-r8a77965.o
>
> 2) without patch
> $ size drivers/pinctrl/renesas/pfc-r8a77965.o
>    text    data     bss     dec     hex filename
>   48191       0       0   48191    bc3f drivers/pinctrl/renesas/pfc-r8a77965.o
>
> Please share your comments.

This looks worthwhile to me, also for other SoCs.

> --- a/drivers/pinctrl/sh-pfc/pfc-r8a77965.c
> +++ b/drivers/pinctrl/sh-pfc/pfc-r8a77965.c
> @@ -18,6 +18,14 @@
>  #include "core.h"
>  #include "sh_pfc.h"
>
> +#ifdef CONFIG_PINCTRL_PFC_R8A77965
> +#define PFC_R8A77965_GROUP     (30)
> +#define PFC_R8A77965_FUNCTION  (4)
> +#else
> +#define PFC_R8A77965_GROUP     (0)
> +#define PFC_R8A77965_FUNCTION  (0)
> +#endif

While introducing the definitions above reduces the overall number of
#ifdefs slightly, it increases separation between the numbers (30 and 4)
and the data they represent...

> +
>  #define CFG_FLAGS (SH_PFC_PIN_CFG_DRIVE_STRENGTH | SH_PFC_PIN_CFG_PULL_UP_DOWN)
>
>  #define CPU_ALL_GP(fn, sfx)                                            \
> @@ -1847,6 +1855,7 @@ static const unsigned int canfd1_data_mux[] = {
>         CANFD1_TX_MARK,         CANFD1_RX_MARK,
>  };
>
> +#ifdef CONFIG_PINCTRL_PFC_R8A77965
>  /* - DRIF0 --------------------------------------------------------------- */
>  static const unsigned int drif0_ctrl_a_pins[] = {
>         /* CLK, SYNC */
> @@ -2120,6 +2129,7 @@ static const unsigned int drif3_data1_b_pins[] = {
>  static const unsigned int drif3_data1_b_mux[] = {
>         RIF3_D1_B_MARK,
>  };
> +#endif
>
>  /* - DU --------------------------------------------------------------------- */
>  static const unsigned int du_rgb666_pins[] = {
> @@ -4380,7 +4390,7 @@ static const unsigned int vin5_clk_mux[] = {
>
>  static const struct {
>         struct sh_pfc_pin_group common[318];
> -       struct sh_pfc_pin_group automotive[30];
> +       struct sh_pfc_pin_group automotive[PFC_R8A77965_GROUP];

Hence I'm more inclined to just do:

    +#ifdef CONFIG_PINCTRL_PFC_R8A77965
            struct sh_pfc_pin_group automotive[30];
    +#endif

here, and move the #ifdef/#endif accordingly.

>  } pinmux_groups = {
>         .common = {
>                 SH_PFC_PIN_GROUP(audio_clk_a_a),
> @@ -4703,6 +4713,7 @@ static const struct {
>                 SH_PFC_PIN_GROUP(vin5_clk),
>         },
>         .automotive = {
> +#ifdef CONFIG_PINCTRL_PFC_R8A77965
>                 SH_PFC_PIN_GROUP(drif0_ctrl_a),
>                 SH_PFC_PIN_GROUP(drif0_data0_a),
>                 SH_PFC_PIN_GROUP(drif0_data1_a),
> @@ -4733,6 +4744,7 @@ static const struct {
>                 SH_PFC_PIN_GROUP(drif3_ctrl_b),
>                 SH_PFC_PIN_GROUP(drif3_data0_b),
>                 SH_PFC_PIN_GROUP(drif3_data1_b),
> +#endif
>         }
>  };

Same for the functions.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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