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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH RFC] pinctrl: sh-pfc: pfc-r8a77965: Optimize pinctrl image
> size for RZ/G2N
> 
> 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.

OK.

> > --- 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...
> 
OK. Will take this out.


> > +
> >  #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.

OK.

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

OK. Will send patches with above changes.

Thanks,
Biju




[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux