From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Warren Subject: Re: [PATCH 3/3] ARM: tegra: refactor tegra{20,30}_boot_secondary Date: Fri, 22 Feb 2013 21:33:32 -0700 Message-ID: <5128469C.2050104@wwwdotorg.org> References: <1361514267-12111-1-git-send-email-josephl@nvidia.com> <1361514267-12111-3-git-send-email-josephl@nvidia.com> <5127B8CC.3030808@wwwdotorg.org> <1361592380.1804.20.camel@jlo-ubuntu-64.nvidia.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1361592380.1804.20.camel-yx3yKKdKkHfc7b1ADBJPm0n48jw8i0AO@public.gmane.org> Sender: linux-tegra-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Joseph Lo Cc: Hiroshi Doyu , "linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org" List-Id: linux-tegra@vger.kernel.org On 02/22/2013 09:06 PM, Joseph Lo wrote: > On Sat, 2013-02-23 at 02:28 +0800, Stephen Warren wrote: >> On 02/21/2013 11:24 PM, Joseph Lo wrote: >>> From: Hiroshi Doyu >>> >>> "tegra_boot_secondary()" has many condition branches for some Tegra >>> SoC generations in a single function so that it's not easy to compile >>> a kernel only for a single SoC if one wants with some reason, debug >>> purpose(?). This patch provides SoC specific version of >>> boot_secondary(), tegra{20,30}_boot_secondary(). This could allow >>> any combination of SoC to be built. Those boot_secondary functions can >>> be preparation when we ntroduce chip specific function pointers in the >>> future without having chip dependent branches around. >>> >>> Also removed unused definition/prototpye. >> >> That "also" is really something that should be a separate patch, since >> it's not related to the code refactoring that's the main purpose of this >> patch. >> >> However, I'll let it slide this time, since I think both patches would >> just end up in Tegra's cleanup branch anyway, even though I did already >> point this out (multiple times?) during downstream review:-( You're >> getting lucky because I don't feel like reviewing this again. >> >> I'll apply this series once I can apply patches for 3.10. >> >> One note to anyone else reading this patch: It does look like this is >> duplicating code that was previously nicely shared in >> tegra_boot_secondary(). However, I believe it's appropriate to do this >> in this case, since the equivalent code for future SoCs (such as >> Tegra114) is extremely different, and so the current shared code won't >> end up being shared, and this would just make tegra_boot_secondary() >> over-complex with conditionals when adding Tegra114 support. > > Hiroshi, > > Per Stephen's comment, should we drop this patch? And refactoring this > later when I add support for Tegra114 CPU PM function. Well I did say it wasn't worth reposting for this. But either way, I wasn't saying anything at all about dropping the patch, just that the patch should have been two separate patches since it really does two separate things. From mboxrd@z Thu Jan 1 00:00:00 1970 From: swarren@wwwdotorg.org (Stephen Warren) Date: Fri, 22 Feb 2013 21:33:32 -0700 Subject: [PATCH 3/3] ARM: tegra: refactor tegra{20,30}_boot_secondary In-Reply-To: <1361592380.1804.20.camel@jlo-ubuntu-64.nvidia.com> References: <1361514267-12111-1-git-send-email-josephl@nvidia.com> <1361514267-12111-3-git-send-email-josephl@nvidia.com> <5127B8CC.3030808@wwwdotorg.org> <1361592380.1804.20.camel@jlo-ubuntu-64.nvidia.com> Message-ID: <5128469C.2050104@wwwdotorg.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 02/22/2013 09:06 PM, Joseph Lo wrote: > On Sat, 2013-02-23 at 02:28 +0800, Stephen Warren wrote: >> On 02/21/2013 11:24 PM, Joseph Lo wrote: >>> From: Hiroshi Doyu >>> >>> "tegra_boot_secondary()" has many condition branches for some Tegra >>> SoC generations in a single function so that it's not easy to compile >>> a kernel only for a single SoC if one wants with some reason, debug >>> purpose(?). This patch provides SoC specific version of >>> boot_secondary(), tegra{20,30}_boot_secondary(). This could allow >>> any combination of SoC to be built. Those boot_secondary functions can >>> be preparation when we ntroduce chip specific function pointers in the >>> future without having chip dependent branches around. >>> >>> Also removed unused definition/prototpye. >> >> That "also" is really something that should be a separate patch, since >> it's not related to the code refactoring that's the main purpose of this >> patch. >> >> However, I'll let it slide this time, since I think both patches would >> just end up in Tegra's cleanup branch anyway, even though I did already >> point this out (multiple times?) during downstream review:-( You're >> getting lucky because I don't feel like reviewing this again. >> >> I'll apply this series once I can apply patches for 3.10. >> >> One note to anyone else reading this patch: It does look like this is >> duplicating code that was previously nicely shared in >> tegra_boot_secondary(). However, I believe it's appropriate to do this >> in this case, since the equivalent code for future SoCs (such as >> Tegra114) is extremely different, and so the current shared code won't >> end up being shared, and this would just make tegra_boot_secondary() >> over-complex with conditionals when adding Tegra114 support. > > Hiroshi, > > Per Stephen's comment, should we drop this patch? And refactoring this > later when I add support for Tegra114 CPU PM function. Well I did say it wasn't worth reposting for this. But either way, I wasn't saying anything at all about dropping the patch, just that the patch should have been two separate patches since it really does two separate things.