All of lore.kernel.org
 help / color / mirror / Atom feed
From: Kukjin Kim <kgene.kim@samsung.com>
To: Tomasz Figa <t.figa@samsung.com>
Cc: Daniel Kurtz <djkurtz@chromium.org>,
	linux-samsung-soc@vger.kernel.org,
	Russell King <linux@arm.linux.org.uk>,
	Heiko Stuebner <heiko@sntech.de>,
	Doug Andersen <dianders@chromium.org>,
	linux-kernel@vger.kernel.org,
	Amit Daniel Kachhap <amit.daniel@samsung.com>,
	kgene.kim@samsung.com, Ben Dooks <ben-linux@fluff.org>,
	Abhilash Kesavan <a.kesavan@samsung.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 1/3] ARM: EXYNOS: pmu: Constify data tables
Date: Tue, 10 Dec 2013 06:00:24 +0900	[thread overview]
Message-ID: <52A62F68.906@samsung.com> (raw)
In-Reply-To: <30145891.cE1204QWiC@amdc1227>

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

WARNING: multiple messages have this Message-ID (diff)
From: kgene.kim@samsung.com (Kukjin Kim)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 1/3] ARM: EXYNOS: pmu: Constify data tables
Date: Tue, 10 Dec 2013 06:00:24 +0900	[thread overview]
Message-ID: <52A62F68.906@samsung.com> (raw)
In-Reply-To: <30145891.cE1204QWiC@amdc1227>

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

  reply	other threads:[~2013-12-09 21:00 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-11-20 18:21 [PATCH 1/3] ARM: EXYNOS: pmu: Constify data tables Daniel Kurtz
2013-11-20 18:21 ` Daniel Kurtz
2013-11-20 18:21 ` [PATCH 2/3] ARM: SAMSUNG: Let s3c_pm_do_restore_*() take const sleep_save Daniel Kurtz
2013-11-20 18:21   ` Daniel Kurtz
2013-12-09 13:27   ` Tomasz Figa
2013-12-09 13:27     ` Tomasz Figa
2013-11-20 18:21 ` [PATCH 3/3] ARM: EXYNOS: Constify clksrc immutable register restore tables Daniel Kurtz
2013-11-20 18:21   ` Daniel Kurtz
2013-12-09 13:14 ` [PATCH 1/3] ARM: EXYNOS: pmu: Constify data tables Tomasz Figa
2013-12-09 13:14   ` Tomasz Figa
2013-12-09 16:11   ` Daniel Kurtz
2013-12-09 16:11     ` Daniel Kurtz
2013-12-09 16:15     ` Tomasz Figa
2013-12-09 16:15       ` Tomasz Figa
2013-12-09 21:00       ` Kukjin Kim [this message]
2013-12-09 21:00         ` Kukjin Kim

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=52A62F68.906@samsung.com \
    --to=kgene.kim@samsung.com \
    --cc=a.kesavan@samsung.com \
    --cc=amit.daniel@samsung.com \
    --cc=ben-linux@fluff.org \
    --cc=dianders@chromium.org \
    --cc=djkurtz@chromium.org \
    --cc=heiko@sntech.de \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-samsung-soc@vger.kernel.org \
    --cc=linux@arm.linux.org.uk \
    --cc=t.figa@samsung.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.