Hi,
On 24/11/16 03:01, Siarhei Siamashka wrote:
On Sun, 20 Nov 2016 14:56:56 +0000 Andre Przywara andre.przywara@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.przywara@arm.com Reviewed-by: Alexander Graf agraf@suse.de Reviewed-by: Chen-Yu Tsai wens@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.