[U-Boot] [PATCH 3/4] arm: Support new Xilinx Zynq platform

Tue Aug 14 20:19:19 CEST 2012

Hi Michal,

On Tue, Aug 14, 2012 at 12:39 PM, Michal Simek <monstr at monstr.eu> wrote:
> On 08/14/2012 07:15 PM, Joe Hershberger wrote:
>>
>> Hi Michal,
>>
>> On Tue, Aug 14, 2012 at 12:11 PM, Michal Simek <monstr at monstr.eu> wrote:
>>>
>>> On 08/14/2012 06:41 PM, Joe Hershberger wrote:
>>>>
>>>>
>>>> Hi Michal,
>>>>
>>>> On Tue, Aug 14, 2012 at 11:28 AM, Michal Simek <monstr at monstr.eu> wrote:
>>>>>
>>>>>
>>>>> On 08/14/2012 05:36 PM, Joe Hershberger wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> Hi Michal,
>>>>>>
>>>>>> On Tue, Aug 14, 2012 at 6:42 AM, Michal Simek <monstr at monstr.eu>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Add timer driver.
>>>>>>>
>>>>>>> Signed-off-by: Michal Simek <monstr at monstr.eu>
>>>>>>> ---
>>>>>>>     arch/arm/cpu/armv7/zynq/Makefile |   48 ++++++++++++
>>>>>>>     arch/arm/cpu/armv7/zynq/timer.c  |  151
>>>>>>> ++++++++++++++++++++++++++++++++++++++
>>>>>>>     2 files changed, 199 insertions(+), 0 deletions(-)
>>>>>>>     create mode 100644 arch/arm/cpu/armv7/zynq/Makefile
>>>>>>>     create mode 100644 arch/arm/cpu/armv7/zynq/timer.c
>>>>>>>
>>>>>>> diff --git a/arch/arm/cpu/armv7/zynq/Makefile
>>>>>>> b/arch/arm/cpu/armv7/zynq/Makefile
>>>>>>> new file mode 100644
>>>>>>> index 0000000..814c1d4
>>>>>>> --- /dev/null
>>>>>>> +++ b/arch/arm/cpu/armv7/zynq/Makefile
>>>>>>> @@ -0,0 +1,48 @@
>>>>>>> +#
>>>>>>> +# (C) Copyright 2000-2003
>>>>>>> +# Wolfgang Denk, DENX Software Engineering, wd at denx.de.
>>>>>>> +#
>>>>>>> +# (C) Copyright 2008
>>>>>>> +# Guennadi Liakhovetki, DENX Software Engineering, <lg at denx.de>
>>>>>>> +#
>>>>>>> +# See file CREDITS for list of people who contributed to this
>>>>>>> +# project.
>>>>>>> +#
>>>>>>> +# This program is free software; you can redistribute it and/or
>>>>>>> +# modify it under the terms of the GNU General Public License as
>>>>>>> +# published by the Free Software Foundation; either version 2 of
>>>>>>> +# the License, or (at your option) any later version.
>>>>>>> +#
>>>>>>> +# This program is distributed in the hope that it will be useful,
>>>>>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>>>>> +# GNU General Public License for more details.
>>>>>>> +#
>>>>>>> +# You should have received a copy of the GNU General Public License
>>>>>>> +# along with this program; if not, write to the Free Software
>>>>>>> +# Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>>>>>>> +# MA 02111-1307 USA
>>>>>>> +#
>>>>>>> +
>>>>>>> +include $(TOPDIR)/config.mk
>>>>>>> +
>>>>>>> +LIB    = $(obj)lib$(SOC).o
>>>>>>> +
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> You should include the lowlevel_init.o here instead of in the board.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Probably make sense.
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>> +COBJS  = timer.o
>>>>>>> +
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Preferably emulate the Makefile in arch/arm/cpu/armv7/tegra2/.  By
>>>>>> that I mean use COBJS-y instead of COBJS directly.  This is more
>>>>>> forward looking to allow for features to be disabled in the future.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> no problem with that.
>>>>>
>>>>>
>>>>>>
>>>>>>> +SRCS   := $(SOBJS:.o=.S) $(COBJS:.o=.c)
>>>>>>> +OBJS   := $(addprefix $(obj),$(SOBJS) $(COBJS))
>>>>>>> +
>>>>>>> +all:   $(obj).depend $(LIB)
>>>>>>> +
>>>>>>> +$(LIB): $(OBJS)
>>>>>>> +       $(call cmd_link_o_target, $(OBJS))
>>>>>>> +
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> +#########################################################################
>>>>>>> +
>>>>>>> +# defines $(obj).depend target
>>>>>>> +include $(SRCTREE)/rules.mk
>>>>>>> +
>>>>>>> +sinclude $(obj).depend
>>>>>>> +
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> +#########################################################################
>>>>>>> diff --git a/arch/arm/cpu/armv7/zynq/timer.c
>>>>>>> b/arch/arm/cpu/armv7/zynq/timer.c
>>>>>>> new file mode 100644
>>>>>>> index 0000000..d79da97
>>>>>>> --- /dev/null
>>>>>>> +++ b/arch/arm/cpu/armv7/zynq/timer.c
>>>>>>> @@ -0,0 +1,151 @@
>>>>>>> +/*
>>>>>>> + * Copyright (C) 2012 Michal Simek <monstr at monstr.eu>
>>>>>>> + * Copyright (C) 2011-2012 Xilinx, Inc. All rights reserved.
>>>>>>> + *
>>>>>>> + * (C) Copyright 2008
>>>>>>> + * Guennadi Liakhovetki, DENX Software Engineering, <lg at denx.de>
>>>>>>> + *
>>>>>>> + * (C) Copyright 2004
>>>>>>> + * Philippe Robin, ARM Ltd. <philippe.robin at arm.com>
>>>>>>> + *
>>>>>>> + * (C) Copyright 2002-2004
>>>>>>> + * Gary Jennejohn, DENX Software Engineering, <gj at denx.de>
>>>>>>> + *
>>>>>>> + * (C) Copyright 2003
>>>>>>> + * Texas Instruments <www.ti.com>
>>>>>>> + *
>>>>>>> + * (C) Copyright 2002
>>>>>>> + * Sysgo Real-Time Solutions, GmbH <www.elinos.com>
>>>>>>> + * Marius Groeger <mgroeger at sysgo.de>
>>>>>>> + *
>>>>>>> + * (C) Copyright 2002
>>>>>>> + * Sysgo Real-Time Solutions, GmbH <www.elinos.com>
>>>>>>> + * Alex Zuepke <azu at sysgo.de>
>>>>>>> + *
>>>>>>> + * See file CREDITS for list of people who contributed to this
>>>>>>> + * project.
>>>>>>> + *
>>>>>>> + * This program is free software; you can redistribute it and/or
>>>>>>> + * modify it under the terms of the GNU General Public License as
>>>>>>> + * published by the Free Software Foundation; either version 2 of
>>>>>>> + * the License, or (at your option) any later version.
>>>>>>> + *
>>>>>>> + * This program is distributed in the hope that it will be useful,
>>>>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.         See
>>>>>>> the
>>>>>>> + * GNU General Public License for more details.
>>>>>>> + *
>>>>>>> + * You should have received a copy of the GNU General Public License
>>>>>>> + * along with this program; if not, write to the Free Software
>>>>>>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>>>>>>> + * MA 02111-1307 USA
>>>>>>> + */
>>>>>>> +
>>>>>>> +#include <common.h>
>>>>>>> +#include <div64.h>
>>>>>>> +#include <asm/io.h>
>>>>>>> +
>>>>>>> +DECLARE_GLOBAL_DATA_PTR;
>>>>>>> +
>>>>>>> +struct scu_timer {
>>>>>>> +       u32 load; /* Timer Load Register */
>>>>>>> +       u32 counter; /* Timer Counter Register */
>>>>>>> +       u32 control; /* Timer Control Register */
>>>>>>> +};
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> You are using the timer in the ARM Cortex A9 core.  This is not
>>>>>> Zynq-specific in any way.  It should be made available in a different
>>>>>> place.  Probably in arch/arm/lib.  It should be stripped on any "XSCU"
>>>>>> references.  Use ARM names instead.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Based on this I can't see the reason why XSCU should be stripped.
>>>>>
>>>>>
>>>>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0407f/BABDEAGJ.html
>>>>>
>>>>> It is SCU private timer. Agree that we can remove X prefix.
>>>>>
>>>>>
>>>>> I don't think that arch/arm/lib is the proper location for that.
>>>>> I am not arm specialist to say that this timer is available in all arm
>>>>> families
>>>>> to be in lib folder.
>>>>
>>>>
>>>>
>>>> I'm not sure that it is available on more than armv7 (like
>>>> arch/arm/lib/cache-pl310.c).  If it is only available in armv7, then
>>>> it can go in arch/arm/cpu/armv7/.
>>>
>>>
>>>
>>> For me was just safer to use it in zynq folder because I am not familiar
>>> with others ARMs.
>>> + I see that other armv7 cpus uses own timer drivers.
>>
>>
>> That is true, but they seem to be using other timer hardware instead
>> of the ARM core timers.  For instance, if you were using the Zynq's
>> Triple Timer Counter module instead, then it would make sense to have
>> this in the zynq directory.
>
>
> Yes, that's partially clear case if Microblaze doesn't want to use it.
>
> Zynq can also use axi_timer. We have this driver in Microblaze folder.

This would be bad because it would assume some timer in the fabric.

> This is driver sharing across architectures. IRC there was any discussion
> about
> moving drivers to generic location in past but this has never happen. (BTW:
> this
> driver can be used by xilinx ppc405 and ppc440)

I guess by "this driver" you mean axi_timer and not scu_timer.

>
>
> If Albert tends to move this driver to any general location I have no
> problem to do
> in spite of there is probably not any other armv7 close which will use it.
>
> From my point of view if there is not other armv7 clone which will use it,
> make sense to keep this in zynq folder. And move it to generic location
> when any other armv7 wants to use.

This is exactly the opposite of what we should be doing.  It is true
that this refactoring has not been very wide-spread yet, but certainly
for new things, the idea is to only put things into a less generic
place if there is no reasonable chance that it will be reused outside
of the more specific scope.  If it is available in other places in
hardware, make it generic.  It's not as good to expect that every new
board should have to search every other board dir in the hope that he
finds a driver that he can reuse.  He should be able to look in the
arch (or even drivers/ if applicable) and complete the search sooner.
Why force extra work to move it later.  To the best of your ability,
put it in the best place.  If this driver is available for all cortex
a9 based SoCs, then chances are every new one added from now on will
use this instead of writing a new one, even if there are other timers
available.

Thanks,
-Joe