Hi, On 24/11/16 03:01, Siarhei Siamashka wrote: > On Sun, 20 Nov 2016 14:56:56 +0000 > Andre Przywara <andre.przyw...@arm.com> wrote: > >> These days many Allwinner SoCs use clock_sun6i.c, although out of them >> only the (original sun6i) A31 has a second MBUS clock register. >> Also setting up the PRCM PLL_CTLR1 register to provide the proper voltage >> seems to be an A31-only feature as well. >> So restrict the initialization to this SoC only to avoid writing bogus >> values to (undefined) registers in other chips. >> >> Signed-off-by: Andre Przywara <andre.przyw...@arm.com> >> Reviewed-by: Alexander Graf <ag...@suse.de> >> Reviewed-by: Chen-Yu Tsai <w...@csie.org> >> --- >> arch/arm/mach-sunxi/clock_sun6i.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/arch/arm/mach-sunxi/clock_sun6i.c >> b/arch/arm/mach-sunxi/clock_sun6i.c >> index ed8cd9b..382fa94 100644 >> --- a/arch/arm/mach-sunxi/clock_sun6i.c >> +++ b/arch/arm/mach-sunxi/clock_sun6i.c >> @@ -21,6 +21,8 @@ void clock_init_safe(void) >> { >> struct sunxi_ccm_reg * const ccm = >> (struct sunxi_ccm_reg *)SUNXI_CCM_BASE; >> + >> +#ifdef CONFIG_MACH_SUN6I >> struct sunxi_prcm_reg * const prcm = >> (struct sunxi_prcm_reg *)SUNXI_PRCM_BASE; >> >> @@ -31,6 +33,7 @@ void clock_init_safe(void) >> PRCM_PLL_CTRL_LDO_DIGITAL_EN | PRCM_PLL_CTRL_LDO_ANALOG_EN | >> PRCM_PLL_CTRL_EXT_OSC_EN | PRCM_PLL_CTRL_LDO_OUT_L(1140)); >> clrbits_le32(&prcm->pll_ctrl1, PRCM_PLL_CTRL_LDO_KEY_MASK); >> +#endif > > PRCM is generally poorly documented, with its description sometimes > entirely missing from the Allwinner manuals (while it exists in the > hardware). But many SoC variants are sharing the same features and need > the same code. I can confirm that this code chunk is needed on my A31s > tablet (otherwise the system does not boot), so it was not entirely > useless.
Sure, in fact I was hoping for people to holler if it breaks their board. > What about the other SoC variants? For example, I can see that the > A23 manual documents this register in a roughly the same way as A31 > (the PLLVDD voltage settings, etc.). But I don't have any A23 hardware > to test. Basically, are we sure that we will not break A23 support > with this commit? > > If I understand it correctly, you primarily wanted to disable this > code on A64. But disabling it everywhere other than A31 may be a > bit too broad. Well, my impression was that this code was added for the A31, and just called clock_sun6i.c because it made sense at this time. Later on people just re-used the _clock_ code because the clocks are compatible, but missed this one - which cares about a regulator, really. So if people can come up with a list of Socs that need this, I am happy to add this to the #ifdef. I just had the impression that boards with AXPs or I2C regulators don't need this. >> >> clock_set_pll1(408000000); >> >> @@ -41,7 +44,9 @@ void clock_init_safe(void) >> writel(AHB1_ABP1_DIV_DEFAULT, &ccm->ahb1_apb1_div); >> >> writel(MBUS_CLK_DEFAULT, &ccm->mbus0_clk_cfg); >> +#ifdef CONFIG_MACH_SUN6I >> writel(MBUS_CLK_DEFAULT, &ccm->mbus1_clk_cfg); >> +#endif > > We can change this to: > > if (IS_ENABLED(CONFIG_MACH_SUN6I)) > writel(MBUS_CLK_DEFAULT, &ccm->mbus1_clk_cfg); > > This saves one line of code and also looks a bit less ugly. Is there some "official" rationale for using IS_ENABLED vs. #ifdef? As much as I dislike this massive usage of #ifdefs, at least it gives me clear heads up that this code may not be compiled in, which can more easily be missed with IS_ENABLED. But I don't have a strong opinion on this, so happy to change it. Cheers, Andre. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot