| Message ID | 1418761900-14035-10-git-send-email-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Delegated to: | Ian Campbell |
| Headers | show |
On Tue, 2014-12-16 at 21:31 +0100, Hans de Goede wrote: > The await_completion helper is already copy pasted between the sun4i and sun6i > dram code, and we need it for sun8i too, so lets make it an inline helper in > dram.h, rather then adding yet another copy. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Acked-by: Ian Campbell <ijc@hellion.org.uk>
On Tue, 16 Dec 2014 21:31:35 +0100 Hans de Goede <hdegoede@redhat.com> wrote: > The await_completion helper is already copy pasted between the sun4i and sun6i > dram code, and we need it for sun8i too, so lets make it an inline helper in > dram.h, rather then adding yet another copy. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Making this function "static inline" and placing it into a header file encourages the compiler to actually inline it. Which is not great for code size: == Before the patch and using gcc version 4.8.3 == $ arm-none-linux-gnieabi-size spl/arch/arm/cpu/armv7/sunxi/dram_sun4i.o text data bss dec hex filename 1731 0 0 1731 6c3 $ arm-none-linux-gnieabi-size spl/arch/arm/cpu/armv7/sunxi/dram_sun6i.o text data bss dec hex filename 1841 0 0 1841 731 == After the patch and using gcc version 4.8.3 == $ arm-none-linux-gnieabi-size spl/arch/arm/cpu/armv7/sunxi/dram_sun4i.o text data bss dec hex filename 1763 0 0 1763 6e3 $ arm-none-linux-gnieabi-size spl/arch/arm/cpu/armv7/sunxi/dram_sun6i.o text data bss dec hex filename 1983 0 0 1983 7bf Could we perhaps just introduce something like a new source file "dram_common.c" or even "dram.c"? > --- > arch/arm/cpu/armv7/sunxi/dram_sun4i.c | 17 ++--------------- > arch/arm/cpu/armv7/sunxi/dram_sun6i.c | 31 +++++++++---------------------- > arch/arm/include/asm/arch-sunxi/dram.h | 14 ++++++++++++++ > 3 files changed, 25 insertions(+), 37 deletions(-) > > diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun4i.c b/arch/arm/cpu/armv7/sunxi/dram_sun4i.c > index ec8aaa7..c736fa3 100644 > --- a/arch/arm/cpu/armv7/sunxi/dram_sun4i.c > +++ b/arch/arm/cpu/armv7/sunxi/dram_sun4i.c > @@ -36,24 +36,11 @@ > #define CPU_CFG_CHIP_REV_B 0x3 > > /* > - * Wait up to 1s for value to be set in given part of reg. > - */ > -static void await_completion(u32 *reg, u32 mask, u32 val) > -{ > - unsigned long tmo = timer_get_us() + 1000000; > - > - while ((readl(reg) & mask) != val) { > - if (timer_get_us() > tmo) > - panic("Timeout initialising DRAM\n"); > - } > -} > - > -/* > * Wait up to 1s for mask to be clear in given reg. > */ > static inline void await_bits_clear(u32 *reg, u32 mask) > { > - await_completion(reg, mask, 0); > + mctl_await_completion(reg, mask, 0); > } > > /* > @@ -61,7 +48,7 @@ static inline void await_bits_clear(u32 *reg, u32 mask) > */ > static inline void await_bits_set(u32 *reg, u32 mask) > { > - await_completion(reg, mask, mask); > + mctl_await_completion(reg, mask, mask); > } > > /* > diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c > index a8bbdfd..e1670e5 100644 > --- a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c > +++ b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c > @@ -27,19 +27,6 @@ struct dram_sun6i_para { > u16 page_size; > }; > > -/* > - * Wait up to 1s for value to be set in given part of reg. > - */ > -static void await_completion(u32 *reg, u32 mask, u32 val) > -{ > - unsigned long tmo = timer_get_us() + 1000000; > - > - while ((readl(reg) & mask) != val) { > - if (timer_get_us() > tmo) > - panic("Timeout initialising DRAM\n"); > - } > -} > - > static void mctl_sys_init(void) > { > struct sunxi_ccm_reg * const ccm = > @@ -51,7 +38,7 @@ static void mctl_sys_init(void) > clrsetbits_le32(&ccm->dram_clk_cfg, CCM_DRAMCLK_CFG_DIV0_MASK, > CCM_DRAMCLK_CFG_DIV0(dram_clk_div) | CCM_DRAMCLK_CFG_RST | > CCM_DRAMCLK_CFG_UPD); > - await_completion(&ccm->dram_clk_cfg, CCM_DRAMCLK_CFG_UPD, 0); > + mctl_await_completion(&ccm->dram_clk_cfg, CCM_DRAMCLK_CFG_UPD, 0); As an additional observation, moving await_bits_clear/await_bits_set functions into a common header file and using them in the sun6i dram code might improve readability. > writel(MDFS_CLK_DEFAULT, &ccm->mdfs_clk_cfg); > > @@ -107,8 +94,8 @@ static bool mctl_rank_detect(u32 *gsr0, int rank) > const u32 done = MCTL_DX_GSR0_RANK0_TRAIN_DONE << rank; > const u32 err = MCTL_DX_GSR0_RANK0_TRAIN_ERR << rank; > > - await_completion(gsr0, done, done); > - await_completion(gsr0 + 0x10, done, done); > + mctl_await_completion(gsr0, done, done); > + mctl_await_completion(gsr0 + 0x10, done, done); > > return !(readl(gsr0) & err) && !(readl(gsr0 + 0x10) & err); > } > @@ -129,7 +116,7 @@ static void mctl_channel_init(int ch_index, struct dram_sun6i_para *para) > } > > writel(MCTL_MCMD_NOP, &mctl_ctl->mcmd); > - await_completion(&mctl_ctl->mcmd, MCTL_MCMD_BUSY, 0); > + mctl_await_completion(&mctl_ctl->mcmd, MCTL_MCMD_BUSY, 0); > > /* PHY initialization */ > writel(MCTL_PGCR, &mctl_phy->pgcr); > @@ -166,14 +153,14 @@ static void mctl_channel_init(int ch_index, struct dram_sun6i_para *para) > writel(MCTL_DX_GCR | MCTL_DX_GCR_EN, &mctl_phy->dx2gcr); > writel(MCTL_DX_GCR | MCTL_DX_GCR_EN, &mctl_phy->dx3gcr); > > - await_completion(&mctl_phy->pgsr, 0x03, 0x03); > + mctl_await_completion(&mctl_phy->pgsr, 0x03, 0x03); > > writel(CONFIG_DRAM_ZQ, &mctl_phy->zq0cr1); > > setbits_le32(&mctl_phy->pir, MCTL_PIR_CLEAR_STATUS); > writel(MCTL_PIR_STEP1, &mctl_phy->pir); > udelay(10); > - await_completion(&mctl_phy->pgsr, 0x1f, 0x1f); > + mctl_await_completion(&mctl_phy->pgsr, 0x1f, 0x1f); > > /* rank detect */ > if (!mctl_rank_detect(&mctl_phy->dx0gsr0, 1)) { > @@ -204,14 +191,14 @@ static void mctl_channel_init(int ch_index, struct dram_sun6i_para *para) > setbits_le32(&mctl_phy->pir, MCTL_PIR_CLEAR_STATUS); > writel(MCTL_PIR_STEP2, &mctl_phy->pir); > udelay(10); > - await_completion(&mctl_phy->pgsr, 0x11, 0x11); > + mctl_await_completion(&mctl_phy->pgsr, 0x11, 0x11); > > if (readl(&mctl_phy->pgsr) & MCTL_PGSR_TRAIN_ERR_MASK) > panic("Training error initialising DRAM\n"); > > /* Move to configure state */ > writel(MCTL_SCTL_CONFIG, &mctl_ctl->sctl); > - await_completion(&mctl_ctl->sstat, 0x07, 0x01); > + mctl_await_completion(&mctl_ctl->sstat, 0x07, 0x01); > > /* Set number of clks per micro-second */ > writel(DRAM_CLK / 1000000, &mctl_ctl->togcnt1u); > @@ -270,7 +257,7 @@ static void mctl_channel_init(int ch_index, struct dram_sun6i_para *para) > > /* Move to access state */ > writel(MCTL_SCTL_ACCESS, &mctl_ctl->sctl); > - await_completion(&mctl_ctl->sstat, 0x07, 0x03); > + mctl_await_completion(&mctl_ctl->sstat, 0x07, 0x03); > } > > static void mctl_com_init(struct dram_sun6i_para *para) > diff --git a/arch/arm/include/asm/arch-sunxi/dram.h b/arch/arm/include/asm/arch-sunxi/dram.h > index 9072e68..18924f5 100644 > --- a/arch/arm/include/asm/arch-sunxi/dram.h > +++ b/arch/arm/include/asm/arch-sunxi/dram.h > @@ -12,6 +12,7 @@ > #ifndef _SUNXI_DRAM_H > #define _SUNXI_DRAM_H > > +#include <asm/io.h> > #include <linux/types.h> > > /* dram regs definition */ > @@ -23,4 +24,17 @@ > > unsigned long sunxi_dram_init(void); > > +/* > + * Wait up to 1s for value to be set in given part of reg. > + */ > +static inline void mctl_await_completion(u32 *reg, u32 mask, u32 val) > +{ > + unsigned long tmo = timer_get_us() + 1000000; > + > + while ((readl(reg) & mask) != val) { > + if (timer_get_us() > tmo) > + panic("Timeout initialising DRAM\n"); > + } > +} > + > #endif /* _SUNXI_DRAM_H */
Hi, On 19-12-14 11:06, Siarhei Siamashka wrote: > On Tue, 16 Dec 2014 21:31:35 +0100 > Hans de Goede <hdegoede@redhat.com> wrote: > >> The await_completion helper is already copy pasted between the sun4i and sun6i >> dram code, and we need it for sun8i too, so lets make it an inline helper in >> dram.h, rather then adding yet another copy. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > Making this function "static inline" and placing it into a header file > encourages the compiler to actually inline it. Which is not great for > code size: > > > == Before the patch and using gcc version 4.8.3 == > > $ arm-none-linux-gnieabi-size spl/arch/arm/cpu/armv7/sunxi/dram_sun4i.o > text data bss dec hex filename > 1731 0 0 1731 6c3 > > $ arm-none-linux-gnieabi-size spl/arch/arm/cpu/armv7/sunxi/dram_sun6i.o > text data bss dec hex filename > 1841 0 0 1841 731 > > == After the patch and using gcc version 4.8.3 == > > $ arm-none-linux-gnieabi-size spl/arch/arm/cpu/armv7/sunxi/dram_sun4i.o > text data bss dec hex filename > 1763 0 0 1763 6e3 > > $ arm-none-linux-gnieabi-size spl/arch/arm/cpu/armv7/sunxi/dram_sun6i.o > text data bss dec hex filename > 1983 0 0 1983 7bf Ah, thanks for catching that, the size increase does not seem to be a problem right now, but it certainly is something to keep in mind. > Could we perhaps just introduce something like a new source file > "dram_common.c" or even "dram.c"? Sounds like a good idea, patches welcome. > As an additional observation, moving await_bits_clear/await_bits_set > functions into a common header file and using them in the sun6i dram > code might improve readability. Sounds like another good idea, patches welcome. Regards, Hans
On Fri, 19 Dec 2014 17:42:58 +0100 Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 19-12-14 11:06, Siarhei Siamashka wrote: > > On Tue, 16 Dec 2014 21:31:35 +0100 > > Hans de Goede <hdegoede@redhat.com> wrote: > > > >> The await_completion helper is already copy pasted between the sun4i and sun6i > >> dram code, and we need it for sun8i too, so lets make it an inline helper in > >> dram.h, rather then adding yet another copy. > >> > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > > Making this function "static inline" and placing it into a header file > > encourages the compiler to actually inline it. Which is not great for > > code size: > > > > > > == Before the patch and using gcc version 4.8.3 == > > > > $ arm-none-linux-gnieabi-size spl/arch/arm/cpu/armv7/sunxi/dram_sun4i.o > > text data bss dec hex filename > > 1731 0 0 1731 6c3 > > > > $ arm-none-linux-gnieabi-size spl/arch/arm/cpu/armv7/sunxi/dram_sun6i.o > > text data bss dec hex filename > > 1841 0 0 1841 731 > > > > == After the patch and using gcc version 4.8.3 == > > > > $ arm-none-linux-gnieabi-size spl/arch/arm/cpu/armv7/sunxi/dram_sun4i.o > > text data bss dec hex filename > > 1763 0 0 1763 6e3 > > > > $ arm-none-linux-gnieabi-size spl/arch/arm/cpu/armv7/sunxi/dram_sun6i.o > > text data bss dec hex filename > > 1983 0 0 1983 7bf > > Ah, thanks for catching that, the size increase does not seem to be a problem > right now, but it certainly is something to keep in mind. > > > Could we perhaps just introduce something like a new source file > > "dram_common.c" or even "dram.c"? > > Sounds like a good idea, patches welcome. I would definitely appreciate if we could have avoided having this unnecessary code size increase in the first place. But if we run out of the code size space, then the oversized static inline functions in the headers are always relatively easy to identify and eliminate in one go. That's only a minor annoyance and I'm not going to waste time on pointless debates. > > As an additional observation, moving await_bits_clear/await_bits_set > > functions into a common header file and using them in the sun6i dram > > code might improve readability. > > Sounds like another good idea, patches welcome.
diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun4i.c b/arch/arm/cpu/armv7/sunxi/dram_sun4i.c index ec8aaa7..c736fa3 100644 --- a/arch/arm/cpu/armv7/sunxi/dram_sun4i.c +++ b/arch/arm/cpu/armv7/sunxi/dram_sun4i.c @@ -36,24 +36,11 @@ #define CPU_CFG_CHIP_REV_B 0x3 /* - * Wait up to 1s for value to be set in given part of reg. - */ -static void await_completion(u32 *reg, u32 mask, u32 val) -{ - unsigned long tmo = timer_get_us() + 1000000; - - while ((readl(reg) & mask) != val) { - if (timer_get_us() > tmo) - panic("Timeout initialising DRAM\n"); - } -} - -/* * Wait up to 1s for mask to be clear in given reg. */ static inline void await_bits_clear(u32 *reg, u32 mask) { - await_completion(reg, mask, 0); + mctl_await_completion(reg, mask, 0); } /* @@ -61,7 +48,7 @@ static inline void await_bits_clear(u32 *reg, u32 mask) */ static inline void await_bits_set(u32 *reg, u32 mask) { - await_completion(reg, mask, mask); + mctl_await_completion(reg, mask, mask); } /* diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c index a8bbdfd..e1670e5 100644 --- a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c +++ b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c @@ -27,19 +27,6 @@ struct dram_sun6i_para { u16 page_size; }; -/* - * Wait up to 1s for value to be set in given part of reg. - */ -static void await_completion(u32 *reg, u32 mask, u32 val) -{ - unsigned long tmo = timer_get_us() + 1000000; - - while ((readl(reg) & mask) != val) { - if (timer_get_us() > tmo) - panic("Timeout initialising DRAM\n"); - } -} - static void mctl_sys_init(void) { struct sunxi_ccm_reg * const ccm = @@ -51,7 +38,7 @@ static void mctl_sys_init(void) clrsetbits_le32(&ccm->dram_clk_cfg, CCM_DRAMCLK_CFG_DIV0_MASK, CCM_DRAMCLK_CFG_DIV0(dram_clk_div) | CCM_DRAMCLK_CFG_RST | CCM_DRAMCLK_CFG_UPD); - await_completion(&ccm->dram_clk_cfg, CCM_DRAMCLK_CFG_UPD, 0); + mctl_await_completion(&ccm->dram_clk_cfg, CCM_DRAMCLK_CFG_UPD, 0); writel(MDFS_CLK_DEFAULT, &ccm->mdfs_clk_cfg); @@ -107,8 +94,8 @@ static bool mctl_rank_detect(u32 *gsr0, int rank) const u32 done = MCTL_DX_GSR0_RANK0_TRAIN_DONE << rank; const u32 err = MCTL_DX_GSR0_RANK0_TRAIN_ERR << rank; - await_completion(gsr0, done, done); - await_completion(gsr0 + 0x10, done, done); + mctl_await_completion(gsr0, done, done); + mctl_await_completion(gsr0 + 0x10, done, done); return !(readl(gsr0) & err) && !(readl(gsr0 + 0x10) & err); } @@ -129,7 +116,7 @@ static void mctl_channel_init(int ch_index, struct dram_sun6i_para *para) } writel(MCTL_MCMD_NOP, &mctl_ctl->mcmd); - await_completion(&mctl_ctl->mcmd, MCTL_MCMD_BUSY, 0); + mctl_await_completion(&mctl_ctl->mcmd, MCTL_MCMD_BUSY, 0); /* PHY initialization */ writel(MCTL_PGCR, &mctl_phy->pgcr); @@ -166,14 +153,14 @@ static void mctl_channel_init(int ch_index, struct dram_sun6i_para *para) writel(MCTL_DX_GCR | MCTL_DX_GCR_EN, &mctl_phy->dx2gcr); writel(MCTL_DX_GCR | MCTL_DX_GCR_EN, &mctl_phy->dx3gcr); - await_completion(&mctl_phy->pgsr, 0x03, 0x03); + mctl_await_completion(&mctl_phy->pgsr, 0x03, 0x03); writel(CONFIG_DRAM_ZQ, &mctl_phy->zq0cr1); setbits_le32(&mctl_phy->pir, MCTL_PIR_CLEAR_STATUS); writel(MCTL_PIR_STEP1, &mctl_phy->pir); udelay(10); - await_completion(&mctl_phy->pgsr, 0x1f, 0x1f); + mctl_await_completion(&mctl_phy->pgsr, 0x1f, 0x1f); /* rank detect */ if (!mctl_rank_detect(&mctl_phy->dx0gsr0, 1)) { @@ -204,14 +191,14 @@ static void mctl_channel_init(int ch_index, struct dram_sun6i_para *para) setbits_le32(&mctl_phy->pir, MCTL_PIR_CLEAR_STATUS); writel(MCTL_PIR_STEP2, &mctl_phy->pir); udelay(10); - await_completion(&mctl_phy->pgsr, 0x11, 0x11); + mctl_await_completion(&mctl_phy->pgsr, 0x11, 0x11); if (readl(&mctl_phy->pgsr) & MCTL_PGSR_TRAIN_ERR_MASK) panic("Training error initialising DRAM\n"); /* Move to configure state */ writel(MCTL_SCTL_CONFIG, &mctl_ctl->sctl); - await_completion(&mctl_ctl->sstat, 0x07, 0x01); + mctl_await_completion(&mctl_ctl->sstat, 0x07, 0x01); /* Set number of clks per micro-second */ writel(DRAM_CLK / 1000000, &mctl_ctl->togcnt1u); @@ -270,7 +257,7 @@ static void mctl_channel_init(int ch_index, struct dram_sun6i_para *para) /* Move to access state */ writel(MCTL_SCTL_ACCESS, &mctl_ctl->sctl); - await_completion(&mctl_ctl->sstat, 0x07, 0x03); + mctl_await_completion(&mctl_ctl->sstat, 0x07, 0x03); } static void mctl_com_init(struct dram_sun6i_para *para) diff --git a/arch/arm/include/asm/arch-sunxi/dram.h b/arch/arm/include/asm/arch-sunxi/dram.h index 9072e68..18924f5 100644 --- a/arch/arm/include/asm/arch-sunxi/dram.h +++ b/arch/arm/include/asm/arch-sunxi/dram.h @@ -12,6 +12,7 @@ #ifndef _SUNXI_DRAM_H #define _SUNXI_DRAM_H +#include <asm/io.h> #include <linux/types.h> /* dram regs definition */ @@ -23,4 +24,17 @@ unsigned long sunxi_dram_init(void); +/* + * Wait up to 1s for value to be set in given part of reg. + */ +static inline void mctl_await_completion(u32 *reg, u32 mask, u32 val) +{ + unsigned long tmo = timer_get_us() + 1000000; + + while ((readl(reg) & mask) != val) { + if (timer_get_us() > tmo) + panic("Timeout initialising DRAM\n"); + } +} + #endif /* _SUNXI_DRAM_H */
The await_completion helper is already copy pasted between the sun4i and sun6i dram code, and we need it for sun8i too, so lets make it an inline helper in dram.h, rather then adding yet another copy. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- arch/arm/cpu/armv7/sunxi/dram_sun4i.c | 17 ++--------------- arch/arm/cpu/armv7/sunxi/dram_sun6i.c | 31 +++++++++---------------------- arch/arm/include/asm/arch-sunxi/dram.h | 14 ++++++++++++++ 3 files changed, 25 insertions(+), 37 deletions(-)