| Message ID | 1416750195-25318-4-git-send-email-hdegoede@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | Ian Campbell |
| Headers | show |
On Sun, 2014-11-23 at 14:43 +0100, Hans de Goede wrote: > The A31s only has one dram channel, so do not bother with trying to initialize > a second channel. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Acked-by: Ian Campbell <ijc@hellion.org.uk>
On Sun, 23 Nov 2014 14:43:13 +0100 Hans de Goede <hdegoede@redhat.com> wrote: > The A31s only has one dram channel, so do not bother with trying to initialize > a second channel. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Ian Campbell has already noticed in the earlier review. In the case *if* the automatic detection works correctly, then why bother adding this *extra* code? Based on your description, it looks like a superfluous band-aid and increases the number of "moving parts". Which generally makes the code less maintainable. There was a talk about calling it a "performance optimization" earlier, but the v2 commit message does not mention this. Neither does it present any benchmark numbers. Basically, it boils down to whether we can trust the automatic detection code to do a proper job or not. Normally, if the automatic detection does not work correctly in all cases, then it needs to be fixed in the long run. But I can clearly understand if you are not willing to take any risks and just want to deploy something that somehow works to the end users sooner. Either way, I have nothing against these patches. Just would prefer a little bit better transparency and more clear commit messages. Keep up the good job.
Hi, On 12-12-14 21:25, Siarhei Siamashka wrote: > On Sun, 23 Nov 2014 14:43:13 +0100 > Hans de Goede <hdegoede@redhat.com> wrote: > >> The A31s only has one dram channel, so do not bother with trying to initialize >> a second channel. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > Ian Campbell has already noticed in the earlier review. In the case *if* > the automatic detection works correctly, It does work correctly in my testing. > then why bother adding this > *extra* code? I'm just mimicking what the boot0 code does here, and the boot0 code does detect the SoC type and not bother with the second channel on A31s, and that seems sensible to do, nothing good will come out of trying to use the second channel on A31s. > Based on your description, it looks like a superfluous > band-aid and increases the number of "moving parts". Which generally > makes the code less maintainable. > > There was a talk about calling it a "performance optimization" earlier, > but the v2 commit message does not mention this. Neither does it present > any benchmark numbers. > > Basically, it boils down to whether we can trust the automatic detection > code to do a proper job or not. Normally, if the automatic detection > does not work correctly in all cases, then it needs to be fixed in the > long run. But I can clearly understand if you are not willing to take > any risks and just want to deploy something that somehow works to the > end users sooner. In my testing the auto-detect code worked fine for automatically disabling the 2nd channel on on A31s, but as said it seems sensible to just not bother with the 2nd channel at all on A31s. Regards, Hans
On Sat, 13 Dec 2014 12:00:44 +0100 Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 12-12-14 21:25, Siarhei Siamashka wrote: > > On Sun, 23 Nov 2014 14:43:13 +0100 > > Hans de Goede <hdegoede@redhat.com> wrote: > > > >> The A31s only has one dram channel, so do not bother with trying to initialize > >> a second channel. > >> > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > > Ian Campbell has already noticed in the earlier review. In the case *if* > > the automatic detection works correctly, > > It does work correctly in my testing. Thanks. Yes, I have seen your confirmation earlier: http://lists.denx.de/pipermail/u-boot/2014-November/196198.html "My assumption was that it would not work, as the A31s has only one channel, or so the datasheets claim. But it turned out it does work" > > then why bother adding this > > *extra* code? > > I'm just mimicking what the boot0 code does here, and the boot0 code does > detect the SoC type and not bother with the second channel on A31s, and that > seems sensible to do, nothing good will come out of trying to use the second > channel on A31s. That's a good point. If boot0 is doing this, then Allwinner developers probably had some valid reason. Too bad that we don't know this reason. You are currently transplanting the Allwinner code into u-boot. And in the short run it is indeed safer to keep it as is. Especially considering that you are apparently not doing any extensive testing of the individual features and hardware registers, but instead heavily relying on assumptions. > > Based on your description, it looks like a superfluous > > band-aid and increases the number of "moving parts". Which generally > > makes the code less maintainable. > > > > There was a talk about calling it a "performance optimization" earlier, > > but the v2 commit message does not mention this. Neither does it present > > any benchmark numbers. > > > > Basically, it boils down to whether we can trust the automatic detection > > code to do a proper job or not. Normally, if the automatic detection > > does not work correctly in all cases, then it needs to be fixed in the > > long run. But I can clearly understand if you are not willing to take > > any risks and just want to deploy something that somehow works to the > > end users sooner. > > In my testing the auto-detect code worked fine for automatically disabling > the 2nd channel on on A31s, but as said it seems sensible to just not bother > with the 2nd channel at all on A31s. In what way is it sensible to have three possible code paths (A31s, A31 with two channels, A31 with a single channel) instead of two before this change (A31/A31s with a single channel, A31 with two channels)? This just results in a worse testing coverage situation.
diff --git a/arch/arm/cpu/armv7/sunxi/Makefile b/arch/arm/cpu/armv7/sunxi/Makefile index 3b6ae47..1337b60 100644 --- a/arch/arm/cpu/armv7/sunxi/Makefile +++ b/arch/arm/cpu/armv7/sunxi/Makefile @@ -10,6 +10,7 @@ obj-y += timer.o obj-y += board.o obj-y += clock.o +obj-y += cpu_info.o obj-y += pinmux.o obj-$(CONFIG_MACH_SUN6I) += prcm.o obj-$(CONFIG_MACH_SUN8I) += prcm.o @@ -21,7 +22,6 @@ obj-$(CONFIG_MACH_SUN7I) += clock_sun4i.o obj-$(CONFIG_MACH_SUN8I) += clock_sun6i.o ifndef CONFIG_SPL_BUILD -obj-y += cpu_info.o ifdef CONFIG_ARMV7_PSCI obj-y += psci.o endif diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c index 30439dc..2ac0b58 100644 --- a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c +++ b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c @@ -372,10 +372,15 @@ unsigned long sunxi_dram_init(void) .rows = 16, }; + /* A31s only has one channel */ + if (sunxi_get_ss_bonding_id() == SUNXI_SS_BOND_ID_A31S) + para.chan = 1; + mctl_sys_init(); mctl_dll_init(0, ¶); - mctl_dll_init(1, ¶); + if (para.chan == 2) + mctl_dll_init(1, ¶); setbits_le32(&mctl_com->ccr, MCTL_CCR_MASTER_CLK_EN | @@ -383,7 +388,9 @@ unsigned long sunxi_dram_init(void) MCTL_CCR_CH1_CLK_EN); mctl_channel_init(0, ¶); - mctl_channel_init(1, ¶); + if (para.chan == 2) + mctl_channel_init(1, ¶); + mctl_com_init(¶); mctl_port_cfg();
The A31s only has one dram channel, so do not bother with trying to initialize a second channel. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- arch/arm/cpu/armv7/sunxi/Makefile | 2 +- arch/arm/cpu/armv7/sunxi/dram_sun6i.c | 11 +++++++++-- 2 files changed, 10 insertions(+), 3 deletions(-)