| Message ID | 1384971686-7208-1-git-send-email-djkurtz@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Daniel, On Thursday 21 of November 2013 02:21:24 Daniel Kurtz wrote: > These tables are all immutable, make them const to save 4416 bytes of RAM. > > size arch/arm/mach-exynos/pmu.o > text data bss > 848 4420 4 // before > 5264 4 4 // after I'm not sure where the mentioned saving of RAM is. Moving data between sections is not supposed to make it use less memory, I believe. Anyway, it's a good practice to mark constant data as const, to disallow changing them at runtime by mistake, so the patch is fine. Except some issues I commented on inline. Reviewed-by: Tomasz Figa <t.figa@samsung.com> Best regards, Tomasz
Hi Tomasz, Thank you for the reviews. On Dec 9, 2013 5:15 AM, "Tomasz Figa" <t.figa@samsung.com> wrote: > > Hi Daniel, > > On Thursday 21 of November 2013 02:21:24 Daniel Kurtz wrote: > > These tables are all immutable, make them const to save 4416 bytes of RAM. > > > > size arch/arm/mach-exynos/pmu.o > > text data bss > > 848 4420 4 // before > > 5264 4 4 // after > > I'm not sure where the mentioned saving of RAM is. Moving data between > sections is not supposed to make it use less memory, I believe. You are correct. This was my misunderstanding from doing too much work with microcontrollers, where .text sections are accessed in place from FLASH for code and const data, but .data memory is copied from a FLASH section to a second RAM section at init for access at runtime. Most modern Linux systems copy/decompress their code and data sections from external storage to RAM anyway, so there is no actual memory savings (except potentially the compiler may be able to optimize a bit more with the const hint). > > Anyway, it's a good practice to mark constant data as const, to disallow > changing them at runtime by mistake, so the patch is fine. Except some > issues I commented on inline. Were there supposed to be inline comments? I don't see any. Best Regards, -djk > > Reviewed-by: Tomasz Figa <t.figa@samsung.com> > > Best regards, > Tomasz >
On Tuesday 10 of December 2013 00:11:40 Daniel Kurtz wrote: > Hi Tomasz, > > Thank you for the reviews. > > On Dec 9, 2013 5:15 AM, "Tomasz Figa" <t.figa@samsung.com> wrote: > > > > Hi Daniel, > > > > On Thursday 21 of November 2013 02:21:24 Daniel Kurtz wrote: > > > These tables are all immutable, make them const to save 4416 bytes of RAM. > > > > > > size arch/arm/mach-exynos/pmu.o > > > text data bss > > > 848 4420 4 // before > > > 5264 4 4 // after > > > > I'm not sure where the mentioned saving of RAM is. Moving data between > > sections is not supposed to make it use less memory, I believe. > > You are correct. This was my misunderstanding from doing too much > work with microcontrollers, where .text sections are accessed in place > from FLASH for code and const data, but .data memory is copied from a > FLASH section to a second RAM section at init for access at runtime. > Most modern Linux systems copy/decompress their code and data sections > from external storage to RAM anyway, so there is no actual memory > savings (except potentially the compiler may be able to optimize a bit > more with the const hint). > > > > > Anyway, it's a good practice to mark constant data as const, to disallow > > changing them at runtime by mistake, so the patch is fine. Except some > > issues I commented on inline. > > Were there supposed to be inline comments? I don't see any. Oops, sorry for this, forgot to remove the last sentence. I initially had one question about the constant pointers below, but I read through the full code again and answered it myself. The patch is fine. Reviewed-by: Tomasz Figa <t.figa@samsung.com> Best regards, Tomasz
On 12/10/13 01:15, Tomasz Figa wrote: > On Tuesday 10 of December 2013 00:11:40 Daniel Kurtz wrote: >> Hi Tomasz, >> >> Thank you for the reviews. >> >> On Dec 9, 2013 5:15 AM, "Tomasz Figa"<t.figa@samsung.com> wrote: >>> >>> Hi Daniel, >>> >>> On Thursday 21 of November 2013 02:21:24 Daniel Kurtz wrote: >>>> These tables are all immutable, make them const to save 4416 bytes of RAM. >>>> >>>> size arch/arm/mach-exynos/pmu.o >>>> text data bss >>>> 848 4420 4 // before >>>> 5264 4 4 // after >>> >>> I'm not sure where the mentioned saving of RAM is. Moving data between >>> sections is not supposed to make it use less memory, I believe. >> >> You are correct. This was my misunderstanding from doing too much >> work with microcontrollers, where .text sections are accessed in place >> from FLASH for code and const data, but .data memory is copied from a >> FLASH section to a second RAM section at init for access at runtime. >> Most modern Linux systems copy/decompress their code and data sections >> from external storage to RAM anyway, so there is no actual memory >> savings (except potentially the compiler may be able to optimize a bit >> more with the const hint). >> >>> >>> Anyway, it's a good practice to mark constant data as const, to disallow >>> changing them at runtime by mistake, so the patch is fine. Except some >>> issues I commented on inline. >> >> Were there supposed to be inline comments? I don't see any. > > Oops, sorry for this, forgot to remove the last sentence. I initially had > one question about the constant pointers below, but I read through the > full code again and answered it myself. > > The patch is fine. > > Reviewed-by: Tomasz Figa<t.figa@samsung.com> > OK, applied 1 to 3 patches into cleanup. Thanks, Kukjin
diff --git a/arch/arm/mach-exynos/pmu.c b/arch/arm/mach-exynos/pmu.c index 97d6885..6609145 100644 --- a/arch/arm/mach-exynos/pmu.c +++ b/arch/arm/mach-exynos/pmu.c @@ -17,9 +17,9 @@ #include "common.h" -static struct exynos_pmu_conf *exynos_pmu_config; +static const struct exynos_pmu_conf *exynos_pmu_config; -static struct exynos_pmu_conf exynos4210_pmu_config[] = { +static const struct exynos_pmu_conf exynos4210_pmu_config[] = { /* { .reg = address, .val = { AFTR, LPA, SLEEP } */ { S5P_ARM_CORE0_LOWPWR, { 0x0, 0x0, 0x2 } }, { S5P_DIS_IRQ_CORE0, { 0x0, 0x0, 0x0 } }, @@ -95,7 +95,7 @@ static struct exynos_pmu_conf exynos4210_pmu_config[] = { { PMU_TABLE_END,}, }; -static struct exynos_pmu_conf exynos4x12_pmu_config[] = { +static const struct exynos_pmu_conf exynos4x12_pmu_config[] = { { S5P_ARM_CORE0_LOWPWR, { 0x0, 0x0, 0x2 } }, { S5P_DIS_IRQ_CORE0, { 0x0, 0x0, 0x0 } }, { S5P_DIS_IRQ_CENTRAL0, { 0x0, 0x0, 0x0 } }, @@ -203,7 +203,7 @@ static struct exynos_pmu_conf exynos4x12_pmu_config[] = { { PMU_TABLE_END,}, }; -static struct exynos_pmu_conf exynos4412_pmu_config[] = { +static const struct exynos_pmu_conf exynos4412_pmu_config[] = { { S5P_ARM_CORE2_LOWPWR, { 0x0, 0x0, 0x2 } }, { S5P_DIS_IRQ_CORE2, { 0x0, 0x0, 0x0 } }, { S5P_DIS_IRQ_CENTRAL2, { 0x0, 0x0, 0x0 } }, @@ -213,7 +213,7 @@ static struct exynos_pmu_conf exynos4412_pmu_config[] = { { PMU_TABLE_END,}, }; -static struct exynos_pmu_conf exynos5250_pmu_config[] = { +static const struct exynos_pmu_conf exynos5250_pmu_config[] = { /* { .reg = address, .val = { AFTR, LPA, SLEEP } */ { EXYNOS5_ARM_CORE0_SYS_PWR_REG, { 0x0, 0x0, 0x2} }, { EXYNOS5_DIS_IRQ_ARM_CORE0_LOCAL_SYS_PWR_REG, { 0x0, 0x0, 0x0} }, @@ -317,7 +317,7 @@ static struct exynos_pmu_conf exynos5250_pmu_config[] = { { PMU_TABLE_END,}, }; -static void __iomem *exynos5_list_both_cnt_feed[] = { +static void __iomem * const exynos5_list_both_cnt_feed[] = { EXYNOS5_ARM_CORE0_OPTION, EXYNOS5_ARM_CORE1_OPTION, EXYNOS5_ARM_COMMON_OPTION, @@ -331,7 +331,7 @@ static void __iomem *exynos5_list_both_cnt_feed[] = { EXYNOS5_TOP_PWR_SYSMEM_OPTION, }; -static void __iomem *exynos5_list_diable_wfi_wfe[] = { +static void __iomem * const exynos5_list_diable_wfi_wfe[] = { EXYNOS5_ARM_CORE1_OPTION, EXYNOS5_FSYS_ARM_OPTION, EXYNOS5_ISP_ARM_OPTION,
These tables are all immutable, make them const to save 4416 bytes of RAM. size arch/arm/mach-exynos/pmu.o text data bss 848 4420 4 // before 5264 4 4 // after Signed-off-by: Daniel Kurtz <djkurtz@chromium.org> --- arch/arm/mach-exynos/pmu.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)