diff mbox

[U-Boot,03/10] sunxi: Introduce a hidden ARCH_SUN6I Kconfig bool

Message ID 1429027621-19252-3-git-send-email-hdegoede@redhat.com
State Superseded
Delegated to: Hans de Goede
Headers show

Commit Message

Hans de Goede April 14, 2015, 4:06 p.m. UTC
sun6i and newer (derived) SoCs such as the sun8i-a23, sun8i-a33 and sun9i
have a various things in common, like having separate ahb reset control
registers, the SID living inside the pmic, custom pmic busses, new style
watchdog, etc.

This commit introduces a new hidden ARCH_SUN6I Kconfig bool which can be
used to check for these features avoiding the need for an ever growing list
of "#if defined CONFIG_MACH_SUN?I" conditionals as we add support for more
"new style" sunxi SoCs.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 arch/arm/cpu/armv7/sunxi/board.c            | 18 +++++++++---------
 arch/arm/cpu/armv7/sunxi/cpu_info.c         |  2 +-
 arch/arm/cpu/armv7/sunxi/usbc.c             |  4 ++--
 arch/arm/include/asm/arch-sunxi/cpu_sun4i.h | 12 ++++++------
 arch/arm/include/asm/arch-sunxi/mmc.h       |  3 +--
 arch/arm/include/asm/arch-sunxi/timer.h     |  8 ++++----
 board/sunxi/Kconfig                         |  9 +++++++++
 board/sunxi/gmac.c                          |  6 +++---
 drivers/mmc/sunxi_mmc.c                     |  3 +--
 drivers/video/sunxi_display.c               | 10 +++++-----
 10 files changed, 41 insertions(+), 34 deletions(-)

Comments

Michal Suchanek April 15, 2015, 6:51 a.m. UTC | #1
On 14 April 2015 at 18:06, Hans de Goede <hdegoede@redhat.com> wrote:
> sun6i and newer (derived) SoCs such as the sun8i-a23, sun8i-a33 and sun9i
> have a various things in common, like having separate ahb reset control
> registers, the SID living inside the pmic, custom pmic busses, new style
> watchdog, etc.
>
> This commit introduces a new hidden ARCH_SUN6I Kconfig bool which can be
> used to check for these features avoiding the need for an ever growing list
> of "#if defined CONFIG_MACH_SUN?I" conditionals as we add support for more
> "new style" sunxi SoCs.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  arch/arm/cpu/armv7/sunxi/board.c            | 18 +++++++++---------
>  arch/arm/cpu/armv7/sunxi/cpu_info.c         |  2 +-
>  arch/arm/cpu/armv7/sunxi/usbc.c             |  4 ++--
>  arch/arm/include/asm/arch-sunxi/cpu_sun4i.h | 12 ++++++------
>  arch/arm/include/asm/arch-sunxi/mmc.h       |  3 +--
>  arch/arm/include/asm/arch-sunxi/timer.h     |  8 ++++----
>  board/sunxi/Kconfig                         |  9 +++++++++
>  board/sunxi/gmac.c                          |  6 +++---
>  drivers/mmc/sunxi_mmc.c                     |  3 +--
>  drivers/video/sunxi_display.c               | 10 +++++-----
>  10 files changed, 41 insertions(+), 34 deletions(-)
>
> diff --git a/arch/arm/cpu/armv7/sunxi/board.c b/arch/arm/cpu/armv7/sunxi/board.c
> index 6471c6b..30d5974 100644
> --- a/arch/arm/cpu/armv7/sunxi/board.c
> +++ b/arch/arm/cpu/armv7/sunxi/board.c
> @@ -173,7 +173,15 @@ void board_init_f(ulong dummy)
>
>  void reset_cpu(ulong addr)
>  {
> -#if defined(CONFIG_MACH_SUN4I) || defined(CONFIG_MACH_SUN5I) || defined(CONFIG_MACH_SUN7I)
> +#ifdef CONFIG_ARCH_SUN6I

Hello,

this looks wrong.

Either this is ARCH_SUNXI and it coverts all or it's ARCH_SUN6I and
then SUN4I and SUN5I should still be checked separately.


> diff --git a/arch/arm/include/asm/arch-sunxi/timer.h b/arch/arm/include/asm/arch-sunxi/timer.h
> index 9a5e488..a6cc443 100644
> --- a/arch/arm/include/asm/arch-sunxi/timer.h
> +++ b/arch/arm/include/asm/arch-sunxi/timer.h
> @@ -67,7 +67,10 @@ struct sunxi_timer_reg {
>         struct sunxi_timer timer[6];    /* We have 6 timers */
>         u8 res2[16];
>         struct sunxi_avs avs;
> -#if defined(CONFIG_MACH_SUN4I) || defined(CONFIG_MACH_SUN5I) || defined(CONFIG_MACH_SUN7I)
> +#ifdef CONFIG_ARCH_SUN6I
ditto

Thanks

Michal
Michal Suchanek April 15, 2015, 7 a.m. UTC | #2
On 14 April 2015 at 18:06, Hans de Goede <hdegoede@redhat.com> wrote:

> --- a/arch/arm/include/asm/arch-sunxi/cpu_sun4i.h
> +++ b/arch/arm/include/asm/arch-sunxi/cpu_sun4i.h
> @@ -37,7 +37,7 @@
>  #define SUNXI_MMC1_BASE                        0x01c10000
>  #define SUNXI_MMC2_BASE                        0x01c11000
>  #define SUNXI_MMC3_BASE                        0x01c12000
> -#if !defined CONFIG_MACH_SUN6I && !defined CONFIG_MACH_SUN8I
> +#ifndef CONFIG_ARCH_SUN6I
>  #define SUNXI_USB0_BASE                        0x01c13000
>  #define SUNXI_USB1_BASE                        0x01c14000
>  #endif
> @@ -45,15 +45,15 @@
>  #define SUNXI_HDMI_BASE                        0x01c16000
>  #define SUNXI_SPI2_BASE                        0x01c17000
>  #define SUNXI_SATA_BASE                        0x01c18000
> -#if !defined CONFIG_MACH_SUN6I && !defined CONFIG_MACH_SUN8I
> +#ifdef CONFIG_ARCH_SUN6I

Also here #ifndef is changed to #ifdef

Thanks

Michal
Hans de Goede April 15, 2015, 7:35 a.m. UTC | #3
Hi,

On 15-04-15 08:51, Michal Suchanek wrote:
> On 14 April 2015 at 18:06, Hans de Goede <hdegoede@redhat.com> wrote:
>> sun6i and newer (derived) SoCs such as the sun8i-a23, sun8i-a33 and sun9i
>> have a various things in common, like having separate ahb reset control
>> registers, the SID living inside the pmic, custom pmic busses, new style
>> watchdog, etc.
>>
>> This commit introduces a new hidden ARCH_SUN6I Kconfig bool which can be
>> used to check for these features avoiding the need for an ever growing list
>> of "#if defined CONFIG_MACH_SUN?I" conditionals as we add support for more
>> "new style" sunxi SoCs.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   arch/arm/cpu/armv7/sunxi/board.c            | 18 +++++++++---------
>>   arch/arm/cpu/armv7/sunxi/cpu_info.c         |  2 +-
>>   arch/arm/cpu/armv7/sunxi/usbc.c             |  4 ++--
>>   arch/arm/include/asm/arch-sunxi/cpu_sun4i.h | 12 ++++++------
>>   arch/arm/include/asm/arch-sunxi/mmc.h       |  3 +--
>>   arch/arm/include/asm/arch-sunxi/timer.h     |  8 ++++----
>>   board/sunxi/Kconfig                         |  9 +++++++++
>>   board/sunxi/gmac.c                          |  6 +++---
>>   drivers/mmc/sunxi_mmc.c                     |  3 +--
>>   drivers/video/sunxi_display.c               | 10 +++++-----
>>   10 files changed, 41 insertions(+), 34 deletions(-)
>>
>> diff --git a/arch/arm/cpu/armv7/sunxi/board.c b/arch/arm/cpu/armv7/sunxi/board.c
>> index 6471c6b..30d5974 100644
>> --- a/arch/arm/cpu/armv7/sunxi/board.c
>> +++ b/arch/arm/cpu/armv7/sunxi/board.c
>> @@ -173,7 +173,15 @@ void board_init_f(ulong dummy)
>>
>>   void reset_cpu(ulong addr)
>>   {
>> -#if defined(CONFIG_MACH_SUN4I) || defined(CONFIG_MACH_SUN5I) || defined(CONFIG_MACH_SUN7I)
>> +#ifdef CONFIG_ARCH_SUN6I
>
> Hello,
>
> this looks wrong.
>
> Either this is ARCH_SUNXI and it coverts all or it's ARCH_SUN6I and
> then SUN4I and SUN5I should still be checked separately.

Look closer, the blocks before / after the #else are swapped to avoid needing
to use #ifndef without good reason as that is ugly.

>
>
>> diff --git a/arch/arm/include/asm/arch-sunxi/timer.h b/arch/arm/include/asm/arch-sunxi/timer.h
>> index 9a5e488..a6cc443 100644
>> --- a/arch/arm/include/asm/arch-sunxi/timer.h
>> +++ b/arch/arm/include/asm/arch-sunxi/timer.h
>> @@ -67,7 +67,10 @@ struct sunxi_timer_reg {
>>          struct sunxi_timer timer[6];    /* We have 6 timers */
>>          u8 res2[16];
>>          struct sunxi_avs avs;
>> -#if defined(CONFIG_MACH_SUN4I) || defined(CONFIG_MACH_SUN5I) || defined(CONFIG_MACH_SUN7I)
>> +#ifdef CONFIG_ARCH_SUN6I

Same here.

Regards,

Hans
Hans de Goede April 15, 2015, 7:36 a.m. UTC | #4
Hi,

On 15-04-15 09:00, Michal Suchanek wrote:
> On 14 April 2015 at 18:06, Hans de Goede <hdegoede@redhat.com> wrote:
>
>> --- a/arch/arm/include/asm/arch-sunxi/cpu_sun4i.h
>> +++ b/arch/arm/include/asm/arch-sunxi/cpu_sun4i.h
>> @@ -37,7 +37,7 @@
>>   #define SUNXI_MMC1_BASE                        0x01c10000
>>   #define SUNXI_MMC2_BASE                        0x01c11000
>>   #define SUNXI_MMC3_BASE                        0x01c12000
>> -#if !defined CONFIG_MACH_SUN6I && !defined CONFIG_MACH_SUN8I
>> +#ifndef CONFIG_ARCH_SUN6I
>>   #define SUNXI_USB0_BASE                        0x01c13000
>>   #define SUNXI_USB1_BASE                        0x01c14000
>>   #endif
>> @@ -45,15 +45,15 @@
>>   #define SUNXI_HDMI_BASE                        0x01c16000
>>   #define SUNXI_SPI2_BASE                        0x01c17000
>>   #define SUNXI_SATA_BASE                        0x01c18000
>> -#if !defined CONFIG_MACH_SUN6I && !defined CONFIG_MACH_SUN8I
>> +#ifdef CONFIG_ARCH_SUN6I
>
> Also here #ifndef is changed to #ifdef

And the same here too.

Regards,

Hans
Michal Suchanek April 15, 2015, 8:01 a.m. UTC | #5
On 15 April 2015 at 09:35, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 15-04-15 08:51, Michal Suchanek wrote:
>>
>> On 14 April 2015 at 18:06, Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> sun6i and newer (derived) SoCs such as the sun8i-a23, sun8i-a33 and sun9i
>>> have a various things in common, like having separate ahb reset control
>>> registers, the SID living inside the pmic, custom pmic busses, new style
>>> watchdog, etc.
>>>
>>> This commit introduces a new hidden ARCH_SUN6I Kconfig bool which can be
>>> used to check for these features avoiding the need for an ever growing
>>> list
>>> of "#if defined CONFIG_MACH_SUN?I" conditionals as we add support for
>>> more
>>> "new style" sunxi SoCs.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>   arch/arm/cpu/armv7/sunxi/board.c            | 18 +++++++++---------
>>>   arch/arm/cpu/armv7/sunxi/cpu_info.c         |  2 +-
>>>   arch/arm/cpu/armv7/sunxi/usbc.c             |  4 ++--
>>>   arch/arm/include/asm/arch-sunxi/cpu_sun4i.h | 12 ++++++------
>>>   arch/arm/include/asm/arch-sunxi/mmc.h       |  3 +--
>>>   arch/arm/include/asm/arch-sunxi/timer.h     |  8 ++++----
>>>   board/sunxi/Kconfig                         |  9 +++++++++
>>>   board/sunxi/gmac.c                          |  6 +++---
>>>   drivers/mmc/sunxi_mmc.c                     |  3 +--
>>>   drivers/video/sunxi_display.c               | 10 +++++-----
>>>   10 files changed, 41 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/arch/arm/cpu/armv7/sunxi/board.c
>>> b/arch/arm/cpu/armv7/sunxi/board.c
>>> index 6471c6b..30d5974 100644
>>> --- a/arch/arm/cpu/armv7/sunxi/board.c
>>> +++ b/arch/arm/cpu/armv7/sunxi/board.c
>>> @@ -173,7 +173,15 @@ void board_init_f(ulong dummy)
>>>
>>>   void reset_cpu(ulong addr)
>>>   {
>>> -#if defined(CONFIG_MACH_SUN4I) || defined(CONFIG_MACH_SUN5I) ||
>>> defined(CONFIG_MACH_SUN7I)
>>> +#ifdef CONFIG_ARCH_SUN6I
>>
>>
>> Hello,
>>
>> this looks wrong.
>>
>> Either this is ARCH_SUNXI and it coverts all or it's ARCH_SUN6I and
>> then SUN4I and SUN5I should still be checked separately.
>
>
> Look closer, the blocks before / after the #else are swapped to avoid
> needing
> to use #ifndef without good reason as that is ugly.

So you mean that

ARCH_SUN6I is
!defined(MACH_SUN4I) && !defined(MACH_SUN5I) && !defined(MACH_SUN7I) &&
(defined(MACH_SUN6I) || defined(MACH_SUN8I) || defined(MACH_SUN9I))

Now that is ugly.

This probably needs a better name then.

Thanks

Michal
Hans de Goede April 15, 2015, 8:07 a.m. UTC | #6
Hi,

On 15-04-15 10:01, Michal Suchanek wrote:
> On 15 April 2015 at 09:35, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>>
>> On 15-04-15 08:51, Michal Suchanek wrote:
>>>
>>> On 14 April 2015 at 18:06, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> sun6i and newer (derived) SoCs such as the sun8i-a23, sun8i-a33 and sun9i
>>>> have a various things in common, like having separate ahb reset control
>>>> registers, the SID living inside the pmic, custom pmic busses, new style
>>>> watchdog, etc.
>>>>
>>>> This commit introduces a new hidden ARCH_SUN6I Kconfig bool which can be
>>>> used to check for these features avoiding the need for an ever growing
>>>> list
>>>> of "#if defined CONFIG_MACH_SUN?I" conditionals as we add support for
>>>> more
>>>> "new style" sunxi SoCs.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>    arch/arm/cpu/armv7/sunxi/board.c            | 18 +++++++++---------
>>>>    arch/arm/cpu/armv7/sunxi/cpu_info.c         |  2 +-
>>>>    arch/arm/cpu/armv7/sunxi/usbc.c             |  4 ++--
>>>>    arch/arm/include/asm/arch-sunxi/cpu_sun4i.h | 12 ++++++------
>>>>    arch/arm/include/asm/arch-sunxi/mmc.h       |  3 +--
>>>>    arch/arm/include/asm/arch-sunxi/timer.h     |  8 ++++----
>>>>    board/sunxi/Kconfig                         |  9 +++++++++
>>>>    board/sunxi/gmac.c                          |  6 +++---
>>>>    drivers/mmc/sunxi_mmc.c                     |  3 +--
>>>>    drivers/video/sunxi_display.c               | 10 +++++-----
>>>>    10 files changed, 41 insertions(+), 34 deletions(-)
>>>>
>>>> diff --git a/arch/arm/cpu/armv7/sunxi/board.c
>>>> b/arch/arm/cpu/armv7/sunxi/board.c
>>>> index 6471c6b..30d5974 100644
>>>> --- a/arch/arm/cpu/armv7/sunxi/board.c
>>>> +++ b/arch/arm/cpu/armv7/sunxi/board.c
>>>> @@ -173,7 +173,15 @@ void board_init_f(ulong dummy)
>>>>
>>>>    void reset_cpu(ulong addr)
>>>>    {
>>>> -#if defined(CONFIG_MACH_SUN4I) || defined(CONFIG_MACH_SUN5I) ||
>>>> defined(CONFIG_MACH_SUN7I)
>>>> +#ifdef CONFIG_ARCH_SUN6I
>>>
>>>
>>> Hello,
>>>
>>> this looks wrong.
>>>
>>> Either this is ARCH_SUNXI and it coverts all or it's ARCH_SUN6I and
>>> then SUN4I and SUN5I should still be checked separately.
>>
>>
>> Look closer, the blocks before / after the #else are swapped to avoid
>> needing
>> to use #ifndef without good reason as that is ugly.
>
> So you mean that
>
> ARCH_SUN6I is
> !defined(MACH_SUN4I) && !defined(MACH_SUN5I) && !defined(MACH_SUN7I) &&
> (defined(MACH_SUN6I) || defined(MACH_SUN8I) || defined(MACH_SUN9I))
>
> Now that is ugly.
>
> This probably needs a better name then.

What this needs is less bikeshedding and you to actually take your time
to properly read the patch.

If you read the help text in the Kconfig file then you will see that

ARCH_SUN6I is set for sun6i derived designs, which all have a number
of things in common, such as having separate ahb reset registers
in the ccm.

Regards,

Hans



>
> Thanks
>
> Michal
>
Michal Suchanek April 15, 2015, 8:45 a.m. UTC | #7
On 15 April 2015 at 10:07, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 15-04-15 10:01, Michal Suchanek wrote:
>>
>> On 15 April 2015 at 09:35, Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> Hi,
>>>
>>>
>>> On 15-04-15 08:51, Michal Suchanek wrote:
>>>>
>>>>
>>>> On 14 April 2015 at 18:06, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>
>>>>>
>>>>> sun6i and newer (derived) SoCs such as the sun8i-a23, sun8i-a33 and
>>>>> sun9i
>>>>> have a various things in common, like having separate ahb reset control
>>>>> registers, the SID living inside the pmic, custom pmic busses, new
>>>>> style
>>>>> watchdog, etc.
>>>>>
>>>>> This commit introduces a new hidden ARCH_SUN6I Kconfig bool which can
>>>>> be
>>>>> used to check for these features avoiding the need for an ever growing
>>>>> list
>>>>> of "#if defined CONFIG_MACH_SUN?I" conditionals as we add support for
>>>>> more
>>>>> "new style" sunxi SoCs.
>>>>>
>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>> ---
>>>>>    arch/arm/cpu/armv7/sunxi/board.c            | 18 +++++++++---------
>>>>>    arch/arm/cpu/armv7/sunxi/cpu_info.c         |  2 +-
>>>>>    arch/arm/cpu/armv7/sunxi/usbc.c             |  4 ++--
>>>>>    arch/arm/include/asm/arch-sunxi/cpu_sun4i.h | 12 ++++++------
>>>>>    arch/arm/include/asm/arch-sunxi/mmc.h       |  3 +--
>>>>>    arch/arm/include/asm/arch-sunxi/timer.h     |  8 ++++----
>>>>>    board/sunxi/Kconfig                         |  9 +++++++++
>>>>>    board/sunxi/gmac.c                          |  6 +++---
>>>>>    drivers/mmc/sunxi_mmc.c                     |  3 +--
>>>>>    drivers/video/sunxi_display.c               | 10 +++++-----
>>>>>    10 files changed, 41 insertions(+), 34 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/cpu/armv7/sunxi/board.c
>>>>> b/arch/arm/cpu/armv7/sunxi/board.c
>>>>> index 6471c6b..30d5974 100644
>>>>> --- a/arch/arm/cpu/armv7/sunxi/board.c
>>>>> +++ b/arch/arm/cpu/armv7/sunxi/board.c
>>>>> @@ -173,7 +173,15 @@ void board_init_f(ulong dummy)
>>>>>
>>>>>    void reset_cpu(ulong addr)
>>>>>    {
>>>>> -#if defined(CONFIG_MACH_SUN4I) || defined(CONFIG_MACH_SUN5I) ||
>>>>> defined(CONFIG_MACH_SUN7I)
>>>>> +#ifdef CONFIG_ARCH_SUN6I
>>>>
>>>>
>>>>
>>>> Hello,
>>>>
>>>> this looks wrong.
>>>>
>>>> Either this is ARCH_SUNXI and it coverts all or it's ARCH_SUN6I and
>>>> then SUN4I and SUN5I should still be checked separately.
>>>
>>>
>>>
>>> Look closer, the blocks before / after the #else are swapped to avoid
>>> needing
>>> to use #ifndef without good reason as that is ugly.
>>
>>
>> So you mean that
>>
>> ARCH_SUN6I is
>> !defined(MACH_SUN4I) && !defined(MACH_SUN5I) && !defined(MACH_SUN7I) &&
>> (defined(MACH_SUN6I) || defined(MACH_SUN8I) || defined(MACH_SUN9I))
>>
>> Now that is ugly.
>>
>> This probably needs a better name then.
>
>
> What this needs is less bikeshedding and you to actually take your time
> to properly read the patch.
>
> If you read the help text in the Kconfig file then you will see that
>
> ARCH_SUN6I is set for sun6i derived designs, which all have a number
> of things in common, such as having separate ahb reset registers
> in the ccm.
>

Sorry, I see the code is technically correct.

However, if the purpose of this patch was to make the code clearer
then it fails at that.

It is unclear even reading the commit message, and that is not
included in the code.

It is not obvious which MACH_SUN?I are ARCH_SUN6I derived. So if you
can come up with a descriptive name for 'a number of things in common,
such as having separate ahb reset registers in the ccm' that's fine
otherwise this obfuscates the code, not clarifies.

That's not to say that simplifying the ifdefs does not have some
value. Just expect 'WTH does SUN7I not select SUN6I?' come up from
time to time.

Thanks

Michal
Ian Campbell April 15, 2015, 7:47 p.m. UTC | #8
On Wed, 2015-04-15 at 10:45 +0200, Michal Suchanek wrote:
> It is not obvious which MACH_SUN?I are ARCH_SUN6I derived. So if you
> can come up with a descriptive name for 'a number of things in common,
> such as having separate ahb reset registers in the ccm' that's fine
> otherwise this obfuscates the code, not clarifies.

I don't particularly object to the patch but this occurred to me too. I
suppose the rule is "first sunxi to look this way".

How about we call groups of similar SoCs a "generation", i.e.
ARCH_SUNXI_GEN2 is what is called ARCH_SUN6I here, meaning GEN1 would be
SUN4/5/7I.

I've deliberately not considered SUN1/2/3I since I don't know much about
them, and we don't support them AFAIK, but perhaps the generation
numbers I've picked should be larger to accommodate them.

Ian.
Hans de Goede April 16, 2015, 7:23 a.m. UTC | #9
Hi,

On 15-04-15 21:47, Ian Campbell wrote:
> On Wed, 2015-04-15 at 10:45 +0200, Michal Suchanek wrote:
>> It is not obvious which MACH_SUN?I are ARCH_SUN6I derived. So if you
>> can come up with a descriptive name for 'a number of things in common,
>> such as having separate ahb reset registers in the ccm' that's fine
>> otherwise this obfuscates the code, not clarifies.
>
> I don't particularly object to the patch but this occurred to me too. I
> suppose the rule is "first sunxi to look this way".
>
> How about we call groups of similar SoCs a "generation", i.e.
> ARCH_SUNXI_GEN2 is what is called ARCH_SUN6I here, meaning GEN1 would be
> SUN4/5/7I.

I like the GEN idea, but not the numbering as it is a bit too arbitrary
how about: ARCH_SUNXI_GEN_SUN6I or (better I think) just SUNXI_GEN_SUN6I ?

I know that does not solve the fact that MACH_SUN7I is SUNXI_GEN_SUN4I
but we cannot fix that, at least this way it will be explicit.

Regards,

Hans
Ian Campbell April 17, 2015, 11:06 a.m. UTC | #10
On Thu, 2015-04-16 at 09:23 +0200, Hans de Goede wrote:
> Hi,
> 
> On 15-04-15 21:47, Ian Campbell wrote:
> > On Wed, 2015-04-15 at 10:45 +0200, Michal Suchanek wrote:
> >> It is not obvious which MACH_SUN?I are ARCH_SUN6I derived. So if you
> >> can come up with a descriptive name for 'a number of things in common,
> >> such as having separate ahb reset registers in the ccm' that's fine
> >> otherwise this obfuscates the code, not clarifies.
> >
> > I don't particularly object to the patch but this occurred to me too. I
> > suppose the rule is "first sunxi to look this way".
> >
> > How about we call groups of similar SoCs a "generation", i.e.
> > ARCH_SUNXI_GEN2 is what is called ARCH_SUN6I here, meaning GEN1 would be
> > SUN4/5/7I.
> 
> I like the GEN idea, but not the numbering as it is a bit too arbitrary
> how about: ARCH_SUNXI_GEN_SUN6I or (better I think) just SUNXI_GEN_SUN6I ?

Works for me (I agree the second is better too).

> 
> I know that does not solve the fact that MACH_SUN7I is SUNXI_GEN_SUN4I
> but we cannot fix that, at least this way it will be explicit.
> 
> Regards,
> 
> Hans
>
diff mbox

Patch

diff --git a/arch/arm/cpu/armv7/sunxi/board.c b/arch/arm/cpu/armv7/sunxi/board.c
index 6471c6b..30d5974 100644
--- a/arch/arm/cpu/armv7/sunxi/board.c
+++ b/arch/arm/cpu/armv7/sunxi/board.c
@@ -173,7 +173,15 @@  void board_init_f(ulong dummy)
 
 void reset_cpu(ulong addr)
 {
-#if defined(CONFIG_MACH_SUN4I) || defined(CONFIG_MACH_SUN5I) || defined(CONFIG_MACH_SUN7I)
+#ifdef CONFIG_ARCH_SUN6I
+	static const struct sunxi_wdog *wdog =
+		 ((struct sunxi_timer_reg *)SUNXI_TIMER_BASE)->wdog;
+
+	/* Set the watchdog for its shortest interval (.5s) and wait */
+	writel(WDT_CFG_RESET, &wdog->cfg);
+	writel(WDT_MODE_EN, &wdog->mode);
+	writel(WDT_CTRL_KEY | WDT_CTRL_RESTART, &wdog->ctl);
+#else
 	static const struct sunxi_wdog *wdog =
 		 &((struct sunxi_timer_reg *)SUNXI_TIMER_BASE)->wdog;
 
@@ -185,14 +193,6 @@  void reset_cpu(ulong addr)
 		/* sun5i sometimes gets stuck without this */
 		writel(WDT_MODE_RESET_EN | WDT_MODE_EN, &wdog->mode);
 	}
-#else /* CONFIG_MACH_SUN6I || CONFIG_MACH_SUN8I || .. */
-	static const struct sunxi_wdog *wdog =
-		 ((struct sunxi_timer_reg *)SUNXI_TIMER_BASE)->wdog;
-
-	/* Set the watchdog for its shortest interval (.5s) and wait */
-	writel(WDT_CFG_RESET, &wdog->cfg);
-	writel(WDT_MODE_EN, &wdog->mode);
-	writel(WDT_CTRL_KEY | WDT_CTRL_RESTART, &wdog->ctl);
 #endif
 }
 
diff --git a/arch/arm/cpu/armv7/sunxi/cpu_info.c b/arch/arm/cpu/armv7/sunxi/cpu_info.c
index b6cb9de..c3ec20d 100644
--- a/arch/arm/cpu/armv7/sunxi/cpu_info.c
+++ b/arch/arm/cpu/armv7/sunxi/cpu_info.c
@@ -76,7 +76,7 @@  int print_cpuinfo(void)
 
 int sunxi_get_sid(unsigned int *sid)
 {
-#if defined CONFIG_MACH_SUN6I || defined CONFIG_MACH_SUN8I
+#ifdef CONFIG_ARCH_SUN6I
 #ifdef CONFIG_AXP221_POWER
 	return axp221_get_sid(sid);
 #else
diff --git a/arch/arm/cpu/armv7/sunxi/usbc.c b/arch/arm/cpu/armv7/sunxi/usbc.c
index 80e4fc9..85b3448 100644
--- a/arch/arm/cpu/armv7/sunxi/usbc.c
+++ b/arch/arm/cpu/armv7/sunxi/usbc.c
@@ -218,7 +218,7 @@  void sunxi_usbc_enable(int index)
 
 	setbits_le32(&ccm->usb_clk_cfg, sunxi_usbc->usb_rst_mask);
 	setbits_le32(&ccm->ahb_gate0, sunxi_usbc->ahb_clk_mask);
-#if defined CONFIG_MACH_SUN6I || defined CONFIG_MACH_SUN8I
+#ifdef CONFIG_ARCH_SUN6I
 	setbits_le32(&ccm->ahb_reset0_cfg, sunxi_usbc->ahb_clk_mask);
 #endif
 
@@ -238,7 +238,7 @@  void sunxi_usbc_disable(int index)
 	if (sunxi_usbc->id != 0)
 		sunxi_usb_passby(sunxi_usbc, !SUNXI_USB_PASSBY_EN);
 
-#if defined CONFIG_MACH_SUN6I || defined CONFIG_MACH_SUN8I
+#ifdef CONFIG_ARCH_SUN6I
 	clrbits_le32(&ccm->ahb_reset0_cfg, sunxi_usbc->ahb_clk_mask);
 #endif
 	clrbits_le32(&ccm->ahb_gate0, sunxi_usbc->ahb_clk_mask);
diff --git a/arch/arm/include/asm/arch-sunxi/cpu_sun4i.h b/arch/arm/include/asm/arch-sunxi/cpu_sun4i.h
index dae6069..3360e87 100644
--- a/arch/arm/include/asm/arch-sunxi/cpu_sun4i.h
+++ b/arch/arm/include/asm/arch-sunxi/cpu_sun4i.h
@@ -37,7 +37,7 @@ 
 #define SUNXI_MMC1_BASE			0x01c10000
 #define SUNXI_MMC2_BASE			0x01c11000
 #define SUNXI_MMC3_BASE			0x01c12000
-#if !defined CONFIG_MACH_SUN6I && !defined CONFIG_MACH_SUN8I
+#ifndef CONFIG_ARCH_SUN6I
 #define SUNXI_USB0_BASE			0x01c13000
 #define SUNXI_USB1_BASE			0x01c14000
 #endif
@@ -45,15 +45,15 @@ 
 #define SUNXI_HDMI_BASE			0x01c16000
 #define SUNXI_SPI2_BASE			0x01c17000
 #define SUNXI_SATA_BASE			0x01c18000
-#if !defined CONFIG_MACH_SUN6I && !defined CONFIG_MACH_SUN8I
+#ifdef CONFIG_ARCH_SUN6I
+#define SUNXI_USB0_BASE			0x01c19000
+#define SUNXI_USB1_BASE			0x01c1a000
+#define SUNXI_USB2_BASE			0x01c1b000
+#else
 #define SUNXI_PATA_BASE			0x01c19000
 #define SUNXI_ACE_BASE			0x01c1a000
 #define SUNXI_TVE1_BASE			0x01c1b000
 #define SUNXI_USB2_BASE			0x01c1c000
-#else
-#define SUNXI_USB0_BASE			0x01c19000
-#define SUNXI_USB1_BASE			0x01c1a000
-#define SUNXI_USB2_BASE			0x01c1b000
 #endif
 #define SUNXI_CSI1_BASE			0x01c1d000
 #define SUNXI_TZASC_BASE		0x01c1e000
diff --git a/arch/arm/include/asm/arch-sunxi/mmc.h b/arch/arm/include/asm/arch-sunxi/mmc.h
index 74833b5..bd1987c 100644
--- a/arch/arm/include/asm/arch-sunxi/mmc.h
+++ b/arch/arm/include/asm/arch-sunxi/mmc.h
@@ -43,8 +43,7 @@  struct sunxi_mmc {
 	u32 chda;		/* 0x90 */
 	u32 cbda;		/* 0x94 */
 	u32 res1[26];
-#if defined(CONFIG_MACH_SUN6I) || defined(CONFIG_MACH_SUN8I) || \
-    defined(CONFIG_MACH_SUN9I)
+#ifdef CONFIG_ARCH_SUN6I
 	u32 res2[64];
 #endif
 	u32 fifo;		/* 0x100 / 0x200 FIFO access address */
diff --git a/arch/arm/include/asm/arch-sunxi/timer.h b/arch/arm/include/asm/arch-sunxi/timer.h
index 9a5e488..a6cc443 100644
--- a/arch/arm/include/asm/arch-sunxi/timer.h
+++ b/arch/arm/include/asm/arch-sunxi/timer.h
@@ -67,7 +67,10 @@  struct sunxi_timer_reg {
 	struct sunxi_timer timer[6];	/* We have 6 timers */
 	u8 res2[16];
 	struct sunxi_avs avs;
-#if defined(CONFIG_MACH_SUN4I) || defined(CONFIG_MACH_SUN5I) || defined(CONFIG_MACH_SUN7I)
+#ifdef CONFIG_ARCH_SUN6I
+	u8 res3[16];
+	struct sunxi_wdog wdog[5];	/* We have 5 watchdogs */
+#else
 	struct sunxi_wdog wdog;	/* 0x90 */
 	/* XXX the following is not accurate for sun5i/sun7i */
 	struct sunxi_64cnt cnt64;	/* 0xa0 */
@@ -77,9 +80,6 @@  struct sunxi_timer_reg {
 	struct sunxi_tgp tgp[4];
 	u8 res5[8];
 	u32 cpu_cfg;
-#else /* CONFIG_MACH_SUN6I || CONFIG_MACH_SUN8I || ... */
-	u8 res3[16];
-	struct sunxi_wdog wdog[5];	/* We have 5 watchdogs */
 #endif
 };
 
diff --git a/board/sunxi/Kconfig b/board/sunxi/Kconfig
index ccc2080..34044f8 100644
--- a/board/sunxi/Kconfig
+++ b/board/sunxi/Kconfig
@@ -1,5 +1,12 @@ 
 if ARCH_SUNXI
 
+config ARCH_SUN6I
+	bool
+	---help---
+	Select this for sunxi SoCs which have sun6i like periphery, like
+	separate ahb reset control registers, custom pmic bus, new style
+	watchdog, etc.
+
 choice
 	prompt "Sunxi SoC Variant"
 
@@ -15,6 +22,7 @@  config MACH_SUN5I
 
 config MACH_SUN6I
 	bool "sun6i (Allwinner A31)"
+	select ARCH_SUN6I
 	select CPU_V7
 	select SUPPORT_SPL
 
@@ -28,6 +36,7 @@  config MACH_SUN7I
 
 config MACH_SUN8I
 	bool "sun8i (Allwinner A23)"
+	select ARCH_SUN6I
 	select CPU_V7
 	select SUPPORT_SPL
 
diff --git a/board/sunxi/gmac.c b/board/sunxi/gmac.c
index 63a7360..2cba891 100644
--- a/board/sunxi/gmac.c
+++ b/board/sunxi/gmac.c
@@ -13,11 +13,11 @@  int sunxi_gmac_initialize(bd_t *bis)
 		(struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
 
 	/* Set up clock gating */
-#ifndef CONFIG_MACH_SUN6I
-	setbits_le32(&ccm->ahb_gate1, 0x1 << AHB_GATE_OFFSET_GMAC);
-#else
+#ifdef CONFIG_ARCH_SUN6I
 	setbits_le32(&ccm->ahb_reset0_cfg, 0x1 << AHB_RESET_OFFSET_GMAC);
 	setbits_le32(&ccm->ahb_gate0, 0x1 << AHB_GATE_OFFSET_GMAC);
+#else
+	setbits_le32(&ccm->ahb_gate1, 0x1 << AHB_GATE_OFFSET_GMAC);
 #endif
 
 	/* Set MII clock */
diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c
index 2233545..fcc278d 100644
--- a/drivers/mmc/sunxi_mmc.c
+++ b/drivers/mmc/sunxi_mmc.c
@@ -151,8 +151,7 @@  static int mmc_clk_io_on(int sdc_no)
 	/* config ahb clock */
 	setbits_le32(&ccm->ahb_gate0, 1 << AHB_GATE_OFFSET_MMC(sdc_no));
 
-#if defined(CONFIG_MACH_SUN6I) || defined(CONFIG_MACH_SUN8I) || \
-    defined(CONFIG_MACH_SUN9I)
+#ifdef CONFIG_ARCH_SUN6I
 	/* unassert reset */
 	setbits_le32(&ccm->ahb_reset0_cfg, 1 << AHB_RESET_OFFSET_MMC(sdc_no));
 #endif
diff --git a/drivers/video/sunxi_display.c b/drivers/video/sunxi_display.c
index d2341b0..e132b75 100644
--- a/drivers/video/sunxi_display.c
+++ b/drivers/video/sunxi_display.c
@@ -84,7 +84,7 @@  static int sunxi_hdmi_hpd_detect(int hpd_delay)
 			CCM_HDMI_CTRL_PLL3);
 
 	/* Set ahb gating to pass */
-#ifdef CONFIG_MACH_SUN6I
+#ifdef CONFIG_ARCH_SUN6I
 	setbits_le32(&ccm->ahb_reset1_cfg, 1 << AHB_RESET_OFFSET_HDMI);
 #endif
 	setbits_le32(&ccm->ahb_gate1, 1 << AHB_GATE_OFFSET_HDMI);
@@ -113,7 +113,7 @@  static void sunxi_hdmi_shutdown(void)
 	clrbits_le32(&hdmi->ctrl, SUNXI_HDMI_CTRL_ENABLE);
 	clrbits_le32(&ccm->hdmi_clk_cfg, CCM_HDMI_CTRL_GATE);
 	clrbits_le32(&ccm->ahb_gate1, 1 << AHB_GATE_OFFSET_HDMI);
-#ifdef CONFIG_MACH_SUN6I
+#ifdef CONFIG_ARCH_SUN6I
 	clrbits_le32(&ccm->ahb_reset1_cfg, 1 << AHB_RESET_OFFSET_HDMI);
 #endif
 	clock_set_pll3(0);
@@ -404,7 +404,7 @@  static void sunxi_composer_init(void)
 
 	sunxi_frontend_init();
 
-#if defined CONFIG_MACH_SUN6I || defined CONFIG_MACH_SUN8I
+#ifdef CONFIG_ARCH_SUN6I
 	/* Reset off */
 	setbits_le32(&ccm->ahb_reset1_cfg, 1 << AHB_RESET_OFFSET_DE_BE0);
 #endif
@@ -549,7 +549,7 @@  static void sunxi_lcdc_init(void)
 		(struct sunxi_lcdc_reg *)SUNXI_LCD0_BASE;
 
 	/* Reset off */
-#if defined CONFIG_MACH_SUN6I || defined CONFIG_MACH_SUN8I
+#ifdef CONFIG_ARCH_SUN6I
 	setbits_le32(&ccm->ahb_reset1_cfg, 1 << AHB_RESET_OFFSET_LCD0);
 #else
 	setbits_le32(&ccm->lcd0_ch0_clk_cfg, CCM_LCD_CH0_CTRL_RST);
@@ -942,7 +942,7 @@  static void sunxi_vga_enable(void)
 
 static void sunxi_drc_init(void)
 {
-#if defined CONFIG_MACH_SUN6I || defined CONFIG_MACH_SUN8I
+#ifdef CONFIG_ARCH_SUN6I
 	struct sunxi_ccm_reg * const ccm =
 		(struct sunxi_ccm_reg *)SUNXI_CCM_BASE;