| Message ID | 1425058685-12956-3-git-send-email-geert+renesas@glider.be |
|---|---|
| State | New |
| Headers | show |
Hi Geert, Thank you for the patch. On Friday 27 February 2015 18:38:03 Geert Uytterhoeven wrote: > Register and field widths are in the range 1..32. Storing them in the > pinctrl data in (arrays of) unsigned long wastes space. > > This decreases the size of a (32-bit) shmobile_defconfig kernel > supporting 7 SoCs by 26460 bytes. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > drivers/pinctrl/sh-pfc/core.c | 2 +- > drivers/pinctrl/sh-pfc/sh_pfc.h | 10 ++++++---- > 2 files changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c > index a56280814a3f884b..466b899ec78b15d7 100644 > --- a/drivers/pinctrl/sh-pfc/core.c > +++ b/drivers/pinctrl/sh-pfc/core.c > @@ -210,7 +210,7 @@ static void sh_pfc_write_config_reg(struct sh_pfc *pfc, > sh_pfc_config_reg_helper(pfc, crp, field, &mapped_reg, &mask, &pos); > > dev_dbg(pfc->dev, "write_reg addr = %lx, value = %ld, field = %ld, " > - "r_width = %ld, f_width = %ld\n", > + "r_width = %u, f_width = %u\n", > crp->reg, value, field, crp->reg_width, crp->field_width); > > mask = ~(mask << pos); > diff --git a/drivers/pinctrl/sh-pfc/sh_pfc.h > b/drivers/pinctrl/sh-pfc/sh_pfc.h index ed5cf4192fa1a2d0..6aeec8152ea674cf > 100644 > --- a/drivers/pinctrl/sh-pfc/sh_pfc.h > +++ b/drivers/pinctrl/sh-pfc/sh_pfc.h > @@ -69,9 +69,10 @@ struct pinmux_func { > }; > > struct pinmux_cfg_reg { > - unsigned long reg, reg_width, field_width; > + unsigned long reg; How about making reg a u32 ? It won't make a difference in practice on 32-bit systems, but it would be more explicit. We could also save space by making reg a u16 and storing the register offset only instead of the full address (assuming it can always fit in 16 bits, which should be checked). We'll also need to support 64-bit systems at some point, and making reg a u64 would increase space waste. > + u8 reg_width, field_width; > const u16 *enum_ids; > - const unsigned long *var_field_width; > + const u8 *var_field_width; > }; > > #define PINMUX_CFG_REG(name, r, r_width, f_width) \ > @@ -80,12 +81,13 @@ struct pinmux_cfg_reg { > > #define PINMUX_CFG_REG_VAR(name, r, r_width, var_fw0, var_fwn...) \ > .reg = r, .reg_width = r_width, \ > - .var_field_width = (const unsigned long [r_width]) \ > + .var_field_width = (const u8 [r_width]) \ > { var_fw0, var_fwn, 0 }, \ > .enum_ids = (const u16 []) > > struct pinmux_data_reg { > - unsigned long reg, reg_width; > + unsigned long reg; > + u8 reg_width; > const u16 *enum_ids; > };
Hi Laurent, On Thu, Mar 5, 2015 at 10:03 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: >> --- a/drivers/pinctrl/sh-pfc/sh_pfc.h >> +++ b/drivers/pinctrl/sh-pfc/sh_pfc.h >> @@ -69,9 +69,10 @@ struct pinmux_func { >> }; >> >> struct pinmux_cfg_reg { >> - unsigned long reg, reg_width, field_width; >> + unsigned long reg; > > How about making reg a u32 ? It won't make a difference in practice on 32-bit > systems, but it would be more explicit. > > We could also save space by making reg a u16 and storing the register offset > only instead of the full address (assuming it can always fit in 16 bits, which > should be checked). We'll also need to support 64-bit systems at some point, > and making reg a u64 would increase space waste. That would be more intrusive (and definitely needs to be in a separate patch), as reg is used here to store a physical register address, for conversion between physical and virtual addresses. I didn't want to go that far yet. u16 would indeed be nice, as it means reg, reg_width, and field_width would fit in one 32-bit word, which I hadn't realized. That means we can reduce each entry by 2 words instead of 1. For 64-bit that would still be suboptimal, as pointers are aligned to 8 bytes, leading to gaps. Perhaps we do want __packed here, too? I don't think the performance drop of doing some unaligned accesses would be significant. This isn't a hot path. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Laurent, On Thu, Mar 5, 2015 at 10:19 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Thu, Mar 5, 2015 at 10:03 AM, Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: >>> --- a/drivers/pinctrl/sh-pfc/sh_pfc.h >>> +++ b/drivers/pinctrl/sh-pfc/sh_pfc.h >>> @@ -69,9 +69,10 @@ struct pinmux_func { >>> }; >>> >>> struct pinmux_cfg_reg { >>> - unsigned long reg, reg_width, field_width; >>> + unsigned long reg; >> >> How about making reg a u32 ? It won't make a difference in practice on 32-bit >> systems, but it would be more explicit. >> >> We could also save space by making reg a u16 and storing the register offset >> only instead of the full address (assuming it can always fit in 16 bits, which >> should be checked). We'll also need to support 64-bit systems at some point, Unfortunately there's a (single) exception: sh7734 has two register blocks, at 0xFFFC0000 and 0xFFC40000, which is more than 64 KiB apart. And we also can't say that all config registers are in the first block, and all data registers are in the second block (for SoCs with multiple blocks). We could use u32 instead of u16 if CONFIG_PINCTRL_PFC_SH7734 is set, though. >> and making reg a u64 would increase space waste. > > That would be more intrusive (and definitely needs to be in a separate patch), > as reg is used here to store a physical register address, for conversion between > physical and virtual addresses. I didn't want to go that far yet. I had a quick check, and using 16-bit register offsets instead of 32-bit registers addresses would save ca. 3 KiB in a shmobile_defconfig kernel. Which is a saving of less than 2% of the current pinctrl binary size. Of course it's something to keep in mind for future 64-bit SoCs... 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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 27, 2015 at 6:38 PM, Geert Uytterhoeven <geert+renesas@glider.be> wrote: > Register and field widths are in the range 1..32. Storing them in the > pinctrl data in (arrays of) unsigned long wastes space. > > This decreases the size of a (32-bit) shmobile_defconfig kernel > supporting 7 SoCs by 26460 bytes. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> This 2/4 is not ACKed so pausing application after 1/4. Is 3,4/4 independent of this patch? Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert, On Thursday 05 March 2015 19:02:10 Geert Uytterhoeven wrote: > On Thu, Mar 5, 2015 at 10:19 AM, Geert Uytterhoeven wrote: > > On Thu, Mar 5, 2015 at 10:03 AM, Laurent Pinchart > > > > <laurent.pinchart@ideasonboard.com> wrote: > >>> --- a/drivers/pinctrl/sh-pfc/sh_pfc.h > >>> +++ b/drivers/pinctrl/sh-pfc/sh_pfc.h > >>> @@ -69,9 +69,10 @@ struct pinmux_func { > >>> }; > >>> > >>> struct pinmux_cfg_reg { > >>> - unsigned long reg, reg_width, field_width; > >>> + unsigned long reg; > >> > >> How about making reg a u32 ? It won't make a difference in practice on > >> 32-bit systems, but it would be more explicit. > >> > >> We could also save space by making reg a u16 and storing the register > >> offset only instead of the full address (assuming it can always fit in > >> 16 bits, which should be checked). We'll also need to support 64-bit > >> systems at some point, > > Unfortunately there's a (single) exception: sh7734 has two register blocks, > at 0xFFFC0000 and 0xFFC40000, which is more than 64 KiB apart. > And we also can't say that all config registers are in the first > block, and all data registers are in the second block (for SoCs with > multiple blocks). > > We could use u32 instead of u16 if CONFIG_PINCTRL_PFC_SH7734 is set, though. Or drop arch/sh support ;-) > >> and making reg a u64 would increase space waste. > > > > That would be more intrusive (and definitely needs to be in a separate > > patch), as reg is used here to store a physical register address, for > > conversion between physical and virtual addresses. I didn't want to go > > that far yet. > > I had a quick check, and using 16-bit register offsets instead of 32-bit > registers addresses would save ca. 3 KiB in a shmobile_defconfig kernel. > Which is a saving of less than 2% of the current pinctrl binary size. > > Of course it's something to keep in mind for future 64-bit SoCs... Let's do that for now then, just keep it in mind.
Hi Geert, On Thursday 05 March 2015 10:19:33 Geert Uytterhoeven wrote: > On Thu, Mar 5, 2015 at 10:03 AM, Laurent Pinchart wrote: > >> --- a/drivers/pinctrl/sh-pfc/sh_pfc.h > >> +++ b/drivers/pinctrl/sh-pfc/sh_pfc.h > >> @@ -69,9 +69,10 @@ struct pinmux_func { > >> }; > >> > >> struct pinmux_cfg_reg { > >> - unsigned long reg, reg_width, field_width; > >> + unsigned long reg; > > > > How about making reg a u32 ? It won't make a difference in practice on > > 32-bit systems, but it would be more explicit. You might have missed this comment. > > We could also save space by making reg a u16 and storing the register > > offset only instead of the full address (assuming it can always fit in 16 > > bits, which should be checked). We'll also need to support 64-bit systems > > at some point, and making reg a u64 would increase space waste. > > That would be more intrusive (and definitely needs to be in a separate > patch), as reg is used here to store a physical register address, for > conversion between physical and virtual addresses. I didn't want to go that > far yet. > > u16 would indeed be nice, as it means reg, reg_width, and field_width > would fit in one 32-bit word, which I hadn't realized. That means we can > reduce each entry by 2 words instead of 1. > > For 64-bit that would still be suboptimal, as pointers are aligned to 8 > bytes, leading to gaps. > Perhaps we do want __packed here, too? I don't think the performance drop of > doing some unaligned accesses would be significant. This isn't a hot path.
Hi Laurent, On Fri, Mar 6, 2015 at 12:05 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Thursday 05 March 2015 10:19:33 Geert Uytterhoeven wrote: >> On Thu, Mar 5, 2015 at 10:03 AM, Laurent Pinchart wrote: >> >> --- a/drivers/pinctrl/sh-pfc/sh_pfc.h >> >> +++ b/drivers/pinctrl/sh-pfc/sh_pfc.h >> >> @@ -69,9 +69,10 @@ struct pinmux_func { >> >> }; >> >> >> >> struct pinmux_cfg_reg { >> >> - unsigned long reg, reg_width, field_width; >> >> + unsigned long reg; >> > >> > How about making reg a u32 ? It won't make a difference in practice on >> > 32-bit systems, but it would be more explicit. > > You might have missed this comment. I intended to, with "reg is used here to store a physical register address", but probably didn't make it sufficiently clear. Before the advent of PAE and phys_addr_t, unsigned long was used to store physical addresses. We could indeed use u32, as the PFC regs are (currently) inside the 32-bit part of the address space. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Linus, Laurent, On Fri, Mar 6, 2015 at 11:48 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Fri, Feb 27, 2015 at 6:38 PM, Geert Uytterhoeven > <geert+renesas@glider.be> wrote: > >> Register and field widths are in the range 1..32. Storing them in the >> pinctrl data in (arrays of) unsigned long wastes space. >> >> This decreases the size of a (32-bit) shmobile_defconfig kernel >> supporting 7 SoCs by 26460 bytes. >> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > This 2/4 is not ACKed so pausing application after 1/4. > > Is 3,4/4 independent of this patch? Unfortunately not, as there are contextual dependencies. Laurent: Is 2/4 OK for you as-is? Your comments about the size of reg are not related to "Store register/field widths in u8 instead of unsigned long", and thus an area for future improvement. Thanks! 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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert, On Friday 06 March 2015 12:21:29 Geert Uytterhoeven wrote: > On Fri, Mar 6, 2015 at 12:05 PM, Laurent Pinchart wrote: > > On Thursday 05 March 2015 10:19:33 Geert Uytterhoeven wrote: > >> On Thu, Mar 5, 2015 at 10:03 AM, Laurent Pinchart wrote: > >> >> --- a/drivers/pinctrl/sh-pfc/sh_pfc.h > >> >> +++ b/drivers/pinctrl/sh-pfc/sh_pfc.h > >> >> @@ -69,9 +69,10 @@ struct pinmux_func { > >> >> > >> >> }; > >> >> > >> >> struct pinmux_cfg_reg { > >> >> > >> >> - unsigned long reg, reg_width, field_width; > >> >> + unsigned long reg; > >> > > >> > How about making reg a u32 ? It won't make a difference in practice on > >> > 32-bit systems, but it would be more explicit. > > > > You might have missed this comment. > > I intended to, with "reg is used here to store a physical register address", > but probably didn't make it sufficiently clear. Before the advent of PAE > and phys_addr_t, unsigned long was used to store physical addresses. > > We could indeed use u32, as the PFC regs are (currently) inside the 32-bit > part of the address space. Sounds good with me. If you want to make that change as a separate patch, Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> for this patch. Otherwise I'll ack v2.
Hi Geert, On Friday 06 March 2015 12:26:57 Geert Uytterhoeven wrote: > On Fri, Mar 6, 2015 at 11:48 AM, Linus Walleij wrote: > > On Fri, Feb 27, 2015 at 6:38 PM, Geert Uytterhoeven wrote: > >> Register and field widths are in the range 1..32. Storing them in the > >> pinctrl data in (arrays of) unsigned long wastes space. > >> > >> This decreases the size of a (32-bit) shmobile_defconfig kernel > >> supporting 7 SoCs by 26460 bytes. > >> > >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > This 2/4 is not ACKed so pausing application after 1/4. > > > > Is 3,4/4 independent of this patch? > > Unfortunately not, as there are contextual dependencies. > > Laurent: Is 2/4 OK for you as-is? Your comments about the size of reg are > not related to "Store register/field widths in u8 instead of unsigned long", > and thus an area for future improvement. As just stated in a different reply to this patch, if you change reg to a u32 in a separate patch (maybe as a v2 of 3/4 ?), Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> for this patch.
diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c index a56280814a3f884b..466b899ec78b15d7 100644 --- a/drivers/pinctrl/sh-pfc/core.c +++ b/drivers/pinctrl/sh-pfc/core.c @@ -210,7 +210,7 @@ static void sh_pfc_write_config_reg(struct sh_pfc *pfc, sh_pfc_config_reg_helper(pfc, crp, field, &mapped_reg, &mask, &pos); dev_dbg(pfc->dev, "write_reg addr = %lx, value = %ld, field = %ld, " - "r_width = %ld, f_width = %ld\n", + "r_width = %u, f_width = %u\n", crp->reg, value, field, crp->reg_width, crp->field_width); mask = ~(mask << pos); diff --git a/drivers/pinctrl/sh-pfc/sh_pfc.h b/drivers/pinctrl/sh-pfc/sh_pfc.h index ed5cf4192fa1a2d0..6aeec8152ea674cf 100644 --- a/drivers/pinctrl/sh-pfc/sh_pfc.h +++ b/drivers/pinctrl/sh-pfc/sh_pfc.h @@ -69,9 +69,10 @@ struct pinmux_func { }; struct pinmux_cfg_reg { - unsigned long reg, reg_width, field_width; + unsigned long reg; + u8 reg_width, field_width; const u16 *enum_ids; - const unsigned long *var_field_width; + const u8 *var_field_width; }; #define PINMUX_CFG_REG(name, r, r_width, f_width) \ @@ -80,12 +81,13 @@ struct pinmux_cfg_reg { #define PINMUX_CFG_REG_VAR(name, r, r_width, var_fw0, var_fwn...) \ .reg = r, .reg_width = r_width, \ - .var_field_width = (const unsigned long [r_width]) \ + .var_field_width = (const u8 [r_width]) \ { var_fw0, var_fwn, 0 }, \ .enum_ids = (const u16 []) struct pinmux_data_reg { - unsigned long reg, reg_width; + unsigned long reg; + u8 reg_width; const u16 *enum_ids; };
Register and field widths are in the range 1..32. Storing them in the pinctrl data in (arrays of) unsigned long wastes space. This decreases the size of a (32-bit) shmobile_defconfig kernel supporting 7 SoCs by 26460 bytes. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- drivers/pinctrl/sh-pfc/core.c | 2 +- drivers/pinctrl/sh-pfc/sh_pfc.h | 10 ++++++---- 2 files changed, 7 insertions(+), 5 deletions(-)