[U-Boot] [PATCH 02/24] sun6i: Restrict some register initialization to Allwinner A31 SoC

[Andre Przywara]

Hi,

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