On 24/11/16 10:18, Andre Przywara wrote:
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.
I now realized that this PLL voltage is obviously generated by an internal regulator, based on the externally provided PLL-Vcc. So in fact this register seems still be valid, even for newer SoCs. But at least for the H2, A64 and H5 I tested this on, the reset value (or the value set by BROM) is exactly 0x00070007, which is what we write here. I think U-Boot shouldn't care about writing those registers if that's the reset value anyway, especially if that happens with a widely used CONFIG symbol. So I will change that #ifdef to just spare the H3 and A64 for now, eventually extending this to more chips, with possibly ending at the A31 being the only user. If any owner of one of those A23, A33, A80 and A83T systems could verify that it works without this register setup (so with this very patch applied), I'd be grateful.
The reset(?) value can be checked via FEL by: $ ./sunxi-fel readl 0x01f01444
Cheers, Andre.
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.