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

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(+)

On Sun, Nov 20, 2016 at 8:26 PM, 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>

Reviewed-by: Jagan Teki <jagan@openedev.com>

thanks!

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.

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.

>  
>  	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.

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.

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.
>